refactor(ai): Inspector AI Assistant Architecture V1 (draft)#357
Draft
dobrinyonkov wants to merge 45 commits into
Draft
refactor(ai): Inspector AI Assistant Architecture V1 (draft)#357dobrinyonkov wants to merge 45 commits into
dobrinyonkov wants to merge 45 commits into
Conversation
…tate Chrome Prompt API rejects 'system' role messages that aren't first in the session. Previously every promptStreaming call sent a fresh [system, ...history, user] array, which broke on the second turn since the session already had internal history. Switch to the API's intended model: - createSession accepts initialPrompts; seed with system prompt + any prior user/assistant turns. - promptStreaming sends only the new user message; session retains history internally. - _loadHistory re-seeds the session after loading stored messages so the model has context on chat reopen. Fixes 'system role message must be the first message of a session' errors after Reset history and on multi-turn conversations.
- _loadHistory: clear _messages before repopulating so re-entry doesn't duplicate every turn (every tab switch was bloating the array). - _loadHistory: re-seed the session even when no stored history exists, so switching to a fresh app context refreshes the system prompt (framework version, theme, libraries) instead of keeping the old one. - _loadHistory: guard against re-entry while streaming or already re-seeding to avoid racing concurrent createSession calls. - _initializeSession: skip empty-content turns so the mid-stream assistant placeholder doesn't get replayed as a blank turn. - handleCreateSession (background): create the new LanguageModel session first and only destroy the old one on success — keeps the previous session usable if init throws.
Introduce a PromptBuilder module that owns the system prompt body, application metadata formatting, selected-control (Inspection Context) formatting, truncation rules, and session seed message construction. PromptBuilder is deterministic and free of Chrome APIs. AISessionManager keeps its public surface unchanged and delegates buildSystemPrompt and user-prompt formatting (inside promptStreaming) to a PromptBuilder instance. The Chrome extension port protocol with the background service worker is unchanged. Prompt-formatting tests migrated from AISessionManager.spec.js to a new PromptBuilder.spec.js. AISessionManager.spec.js retains only transport/session assertions.
…undary
Carve the local AI transport (chrome.runtime.connect({ name: 'prompt-api' }),
port message types, streaming async iterator, capability/usage/disconnect
handling) into a new PromptClient module. PromptClient consumes already-built
prompts and seed messages from PromptBuilder and does not perform prompt
construction itself.
Reduce AISessionManager to a thin facade composing PromptBuilder + PromptClient
so AIChat continues to work with no observable behavior change. The Chrome
extension port contract with the background service worker is unchanged.
Migrate AISessionManager.spec.js from internal-field assertions onto
external-behavior delegation tests. Add transport-level PromptClient.spec.js
covering capability state, download progress, session creation, streaming
chunks, usage info, mid-stream error, and mid-stream disconnect, all driven
through a deterministic fake port.
Issue: .scratch/ai-assistant-architecture-v1/issues/02-reshape-prompt-client.md
Captures the domain model and refactor plan for Inspector AI Assistant V1: - CONTEXT.md: canonical glossary (Inspector AI Assistant, Prompt Builder, Prompt Client, Conversation Store, Assistant Controller, etc.) - docs/adr/0001: incremental refactor decision and target boundaries - docs/agents/: issue tracker, triage labels, domain doc conventions - .scratch/ai-assistant-architecture-v1/: PRD and 6 tracer-bullet issues - prompt.md: TDD-flavored runner prompt for autonomous slices No production code changes.
Wrap the existing chat storage logic in a named ConversationStore interface that owns Conversation Memory: load, append, clear, retention limit (50), and storage-key shape per inspected URL. ChatStorageManager becomes a thin delegating shim so AIChat behavior is unchanged. No other module accesses chrome.storage directly for chat history after this slice. Migrates the legacy ChatStorageManager spec onto ConversationStore's public surface; adds tests pinning the retention limit and the role+content-only persistence rule (Inspection Context must not leak into Conversation Memory). Refs: .scratch/ai-assistant-architecture-v1/issues/03-introduce-conversation-store.md
Issue 02 (Reshape session manager into Prompt Client interface) and issue 03 (Introduce Conversation Store boundary) are both shipped: PromptClient owns the prompt-api port transport, ConversationStore owns chrome.storage-backed Conversation Memory, and AISessionManager / ChatStorageManager remain only as thin delegating shims for AIChat. [INTERNAL]
… view Carve out the Assistant Controller boundary so the Inspector AI Assistant workflow lives in a single named place. AssistantController depends only on PromptBuilder, PromptClient, and ConversationStore and owns: - Assistant Capability State resolution (ready, downloadable, downloading, unsupported, unavailable, streaming-failed, unknown — definitive vocabulary finalized in slice 05) - Conversation Memory loading per inspected URL - session creation and reseed with system prompt + replayed turns - per-turn Inspection Context injection (never persisted) - streaming send, save of both turns, streaming-failure recovery - clear-and-reseed and reseed-on-URL-change AIChat is now a thin view over the controller: rendering, markdown, JSON / code viewers, dialogs, scroll behavior, and clipboard helpers stay; session lifecycle, streaming orchestration, history persistence, and context injection are gone from the view and replaced by controller calls plus an event-listener wiring (capability-state-changed, conversation-loaded, stream-chunk, stream-complete, stream-failed, conversation-cleared). A new AssistantController.spec exercises the full V1 Agent Validation Loop with deterministic fakes for PromptClient and ConversationStore and the real PromptBuilder, with no Chrome runtime dependency. Existing DOM-based AIChat tests continue to pass unchanged. [INTERNAL]
… semantics Address standards and spec review findings on the slice 04 commit. Spec fixes (slice 04 / PRD): - sendUserMessage no longer appends the user turn to ConversationStore before the stream completes. On streaming failure, neither the user nor the assistant turn is persisted, so reseed never replays an orphan user message. Matches slice 04 "saving completed user/assistant turns". - session creation failures during initialize, clearConversation, setUrl, and downloadModel reseed now resolve the Assistant Capability State to 'session-failed' instead of throwing. session-failed is one of the canonical first-class states listed in the PRD; it is no longer just an unhandled rejection. - streaming-failed now recovers: a subsequent successful sendUserMessage resurfaces a 'ready' capability state so the developer does not stay stuck on a failure banner (PRD user story #8). - AIChat capability-state routing is explicit per state; 'unknown' and 'streaming-failed' no longer fall through to the unavailable banner, and 'session-failed' surfaces clearly. Refactored the dispatch into a small prototype lookup map to keep cyclomatic complexity within JSHint limits and to make slice 05 additions trivial. Standards / readability: - AssistantController#on JSDoc now justifies the local in-process event bus and explains why this is not a chrome.runtime message dispatch. - AIChat conversation-cleared handler has a comment confirming the innerHTML literal is static, with user/assistant content always routed through _escapeHtml or _parseMarkdown via _addMessage. - Hoisted duplicated test helpers (initializedReady, awaitStreamController) to module scope with JSDoc. 416 -> 421 tests, 5 new tests covering the spec fixes above. grunt lint and grunt test green. [INTERNAL]
…patch Address follow-up review findings on the slice 04 hardening commit. Spec fixes: - Seed Assistant Capability State as the canonical PRD value 'unavailable' (with a 'Checking model status...' message) instead of the ad-hoc string 'unknown'. The PRD enumerates exactly seven first-class capability states and 'unknown' was not one of them; this brings the controller fully in line with the canonical vocabulary before slice 05. - AIChat capability-state dispatch now has an explicit fallback for unmapped Assistant Capability States: it routes to the unavailable banner with the controller's supplied message and logs a console.warn so a missing handler is detectable during development. Previously an unmapped state was silently dropped, which weakened the PRD guarantee that capability states are first-class. Standards / readability: - Extracted each entry of _capabilityStateHandlers into its own documented AIChat.prototype._render*CapabilityState method; the map now holds references rather than anonymous functions, so each renderer has full JSDoc and a stable name in stack traces. - awaitStreamController helper is now bounded by a maxAttempts polling budget (default 500 x 1ms); if production code never calls promptStreaming, the helper rejects with a descriptive error instead of hanging until Mocha's suite-level timeout. 421 -> 422 tests, 1 new test covering the unmapped-state fallback, 1 existing test rewritten to assert canonical PRD vocabulary at construction. grunt lint and grunt test green. [INTERNAL]
Slice 04 (Introduce Assistant Controller and the V1 Agent Validation Loop) is shipped: - AssistantController owns capability resolution, conversation memory lifecycle, session creation and reseed, per-turn Inspection Context injection, streaming, persistence of completed turns, and clear / reseed semantics. It depends only on PromptBuilder, PromptClient, and ConversationStore. - AIChat no longer owns session lifecycle, streaming orchestration, history persistence, or context injection; it interacts only with the controller and listens to its event stream. - The full V1 Agent Validation Loop is exercised by 22 controller-level tests using deterministic fakes for PromptClient and ConversationStore and the real PromptBuilder — no Chrome runtime dependency. - Existing DOM-based AIChat tests still pass; a new view-level test guards against silent-drop of unmapped capability states. Implementing commits: b846529, 5f0530d, 3675be1. [INTERNAL]
Incorporates master's ec1befd (PR #350: fix(ai): seed system+history via initialPrompts; let session manage state). Master's hunks targeted code that has been refactored into PromptBuilder, PromptClient, and AssistantController on this branch; the conflicts were resolved by keeping the post-refactor shape and verifying that both behavior improvements from PR #350 are preserved in their new homes: - background/main.js handleCreateSession still creates the new LanguageModel session before destroying the prior one, so a failure during init leaves the previous session usable. - PromptBuilder.buildSeedMessages still skips empty assistant placeholders when replaying Conversation Memory, so an interrupted mid-stream slot never reseeds as a blank turn. Adds two tests exercising these behaviors at the right boundaries: - AssistantController.spec.js: empty assistant placeholders from Conversation Memory are not replayed into the session seed. - PromptClient.spec.js: a failed second createSession leaves hasActiveSession() === true so the prior background session can remain usable. See .scratch/ai-assistant-architecture-v1/issues/08-merge-master-preserve-behavior.md.
- Remove dead _isReseedingSession field from AIChat that landed via the auto-merge from master. The view no longer owns reseed coordination; the controller does. The field had no reader, no other writer, and no test exercising it. - Shorten the new Assistant Controller test title to match the local imperative-short convention used elsewhere in the file. - Tighten the new Prompt Client create-before-destroy companion test: use async/await sequencing instead of nested .then chains so the test is no longer coupled to the synchronous registration order inside the Promise executor. Known gap (documented as follow-up, out of scope for the merge issue): the actual create-before-destroy ordering invariant lives in background/main.js handleCreateSession, which sits inside the service worker IIFE and is not unit-testable without restructuring (forbidden by issue 08). The added Prompt Client test still covers the client-side companion invariant — that PromptClient does not optimistically clear hasActiveSession on a failed reseed — which is the strongest assertion available from the testable boundary.
Drop the view-private translation table (status-needs-download, status-error) and the per-state handler map. The AIChat banner now derives its CSS class directly from the controller's canonical status name (status-<status>) for every Assistant Capability State, and the download button is bound to the two states that admit it. AssistantController.initialize() now maps a rejected Prompt Client capability check onto an 'unavailable' canonical state instead of letting the promise reject, so the view never has to render an ad-hoc 'error' banner from a raw exception. session-failed gets its own LESS rule (recoverable warning) and the view re-exposes the clear-history affordance for it, making the user-facing recovery action visible. streaming-failed continues to leave the prior banner untouched per PRD user story #8; tests now make that contract explicit rather than relying on the default download-button visibility. Closes slice 05 of .scratch/ai-assistant-architecture-v1.
Close slice 06: trim AIChat to a thin view over AssistantController.
Most of the trimming had already landed across slices 04, 05 and 08.
This commit closes the two remaining boundary gaps:
1. AIChat tests no longer depend on real session, storage, or capability
behavior. The top-level beforeEach previously built the view with a
real AssistantController, which in turn built a real ConversationStore
that requires chrome.storage.local. The suite only happened to pass
in the full Karma run because other specs leak window.chrome onto the
shared test window; running tests/modules/ui/AIChat.spec.js in
isolation crashed in the beforeEach. Hoist createFakeController() to
the top of the spec and have every beforeEach build the view with
{ controller: fakeController }. The view is now tested at its true
seam — the controller surface — and the spec passes in isolation.
2. Removed three dead view-side fields that suggested workflow ownership:
_currentContext (write-only), _messages (write-only), and _currentUrl
(only used to dedupe setUrl calls that the controller already
short-circuits). Their presence read like the view holds Inspection
Context, Conversation Memory, and URL state when it does not. Dropped
the matching internal-state assertions from Constructor &
Initialization.
No production behavior change. The public surface used by panel/ui5/main.js
(setUrl, updateContext, onTabActivated, destroy) is unchanged. Full
grunt test green (435 tests).
The async iterator returned by promptStreaming() previously wired its chunk / complete / error handlers lazily, only when 'for await' (or iterator.next()) was first called. Real port messages arriving between '_send(prompt-streaming)' and the first iteration were dropped. Today this only happened to be safe because the AIChat 'for await' loop begins on the same microtask that resolves the returned promise; any UI change that deferred iteration (a microtask gap, a wrapping helper, a debounce) would silently lose the first chunks. Move the handlers and the in-memory buffer to be created synchronously inside promptStreaming(), before the returned promise resolves. The async iterator now only drains from the buffer; transport listeners are never registered from inside the generator. Streaming becomes order-independent: chunks delivered before the first iterator.next() call are buffered and replayed in order. Add a deterministic test on the PromptClient boundary that asserts buffered delivery. Existing chunk-yield, mid-stream error, and disconnect tests continue to pass unchanged. AISessionManager and AIChat are not modified. No observable behavior change in the Inspector AI Assistant tab. Closes .scratch/ai-assistant-architecture-v1/issues/07-prewire-streaming-buffer.md
Delete the AISessionManager and ChatStorageManager facades — both had zero production callers; only their own spec files depended on them. The V1 boundaries (PromptBuilder, PromptClient, ConversationStore, AssistantController) are now the only assistant modules, matching what ADR-0001 described as the post-transition end state. Fix the latent capability-mapping gap in AssistantController._mapAvailability: the background service worker can emit 'downloading' (when the local model is mid-download at the moment of the availability check) and 'error' (when the availability check itself throws). Before this change, both collapsed silently into 'unavailable'. After this change, 'downloading' maps to the canonical 'downloading' Assistant Capability State so the AI tab shows the download-in-progress banner instead of a generic unavailable message; 'error' continues to map to 'unavailable' but the transport-supplied message is preserved (this was already the fallback behavior — the spec now guards it). The mapping fix is done in place for this slice. Slice 02 of V2 will move the translation behind the Prompt Client seam. Closes .scratch/ai-assistant-architecture-v2/issues/01-close-adr-0001-and-fix-capability-mapping.md
…nt seam Move the canonical Assistant Capability State translation from AssistantController._mapAvailability into PromptClient.checkAvailability. The Prompt Client now returns one of the canonical state names defined in CONTEXT.md (unsupported, unavailable, downloadable, downloading, ready) directly, instead of leaking the background service worker's port-protocol status dialect (ready, needs-download, downloading, unsupported, unavailable, error) past its boundary. The background port message protocol itself is unchanged. The redundant 'available' boolean is dropped from the checkAvailability return shape; its only consumer was the controller's translation, which also goes away. _mapAvailability is removed from AssistantController. Behavior-preserving: the same canonical state reaches the AIChat view, just produced one seam earlier. Slice 01's mapping fix for the downloading and error background statuses is preserved. The Prompt Client spec now asserts the canonical name returned for every background-port status it can receive, including the error and unrecognized-status paths. The Assistant Controller spec drives its fake Prompt Client with canonical state names directly so the test double does not bookkeep a translation table. Closes ai-assistant-architecture-v2 slice 02.
AIChat used to own about half its 1118 lines on markdown parsing, JSON viewer rendering, code viewer rendering, scroll bookkeeping, debounced streaming render, clipboard helpers, and HTML escaping. The AIChat spec probed fourteen private rendering helpers, which was the strongest internal-seam-wants-to-be-a-module signal in the assistant code. Lift those concerns into a new named AssistantTranscript module in modules/ai. The view now drives the transcript through a small stream-shaped interface — appendUserTurn, appendSystemMessage, beginAssistantTurn (returning a streamChunk/finalize handle), clear, reset, scrollToBottom — and no longer reaches past it. AIChat keeps the banner, input area, confirm dialog, token counter, and controller subscription. The Assistant Controller is unchanged and is unaware the Transcript exists. Migrate the view-private rendering test probes to a new AssistantTranscript spec that asserts behavior through the module's interface, and trim the AIChat spec to view-level assertions (controller-event wiring, banner CSS class from canonical capability states, input/send wiring, dialog focus management, token-counter behavior). CONTEXT.md adds 'Assistant Transcript' to the glossary with an _Avoid_ list excluding chat history, conversation memory, and AIChat. Closes slice 03 of Inspector AI Assistant architecture v2.
- Drop AssistantController#isStreaming() and PromptClient#hasActiveSession(), neither of which had a production caller. Tests now read the underlying state via the underscore-prefixed private members. - Remove unused promptBuilder field from AssistantController.spec harness. - Remove unused streamingHandle field from AIChat.spec fake transcript. - Extract createClient() helper to collapse 17 duplicated test preambles in PromptClient.spec.
Drop references to removed agent workflow artifacts (PRD, ADR, slice issues). Keep the rationale, drop the citation. Trim verbose JSDoc to one-line descriptions where the function name was already self-evident. Remove restating inline comments. Preserve security notes, ordering constraints, and other non-obvious why-comments.
Remove JSDoc descriptions that just restate the function name. Keep the JSDoc block and all tags so ESLint still passes. Delete a handful of inline test-flow markers and one stale historical migration note.
Replace every `var` with `const` (or `let` where reassigned) across the new AI modules and their specs. Replace the pre-existing 'self' aliases in AssistantTranscript with 'that' to match the repo's consistent-this rule. Add `no-var` and `prefer-const` to eslintrc as warnings so future contributions get a signal without breaking the build for existing files that still use `var`.
The 'const that = this' pattern predates arrow functions. Convert every inner callback in AssistantController and AssistantTranscript to an arrow function, drop the alias, and use 'this' directly. 17 aliases removed, 34 inner callbacks converted. Outer prototype methods keep their 'function' form because they rely on dynamic 'this' binding.
Apply the writing style guide to every JSDoc description and inline comment block across the AI modules and their specs. Drop filler openings, hedging language, redundant second sentences, semicolons in prose, and AI-cliche verbs (utilize, leverage, comprehensive, etc.). Re-wrap JSDoc prose at 100 chars. Removes ~180 lines of comment prose without losing any documented constraint, security note, or ordering rule.
Replace the `options = options || {}; this._x = options.x || default;`
pattern with parameter-list destructuring across the 5 new AI module
constructors. The defaults are now visible at the function signature.
Kept AssistantTranscript's maxJsonDepth and streamDebounceMs outside the
destructure because the original guard rejected non-numbers, which the
destructuring default doesn't replicate.
Replace AIChat._CANONICAL_CAPABILITY_STATES (array) and the chain of if-status-equals branches in _onCapabilityStateChanged with a single config object. Each canonical state maps to a flags object indicating whether the dispatch skips the banner, shows the clear-history button, or refreshes the token counter. Same behaviour, fewer branches to read.
The fake PromptClient in AssistantController.spec.js had a 50-line hand-rolled stream object that implemented Symbol.asyncIterator manually with a notify callback. Replace with an async generator that loops over the same chunks/done/error queue. Same external behaviour, fewer moving parts.
A JSDoc block whose only content is `@private` carries no information beyond what the underscore prefix already conveys. Remove 29 such blocks across AssistantTranscript, PromptClient, and AIChat. JSDoc blocks with @param / @returns / actual prose are kept.
No production caller. The view subscribes to capability-state-changed events instead of polling. Tests now read controller._capabilityState directly, consistent with how _isStreaming and PromptClient._hasActiveSession are tested.
Wire AIChat.destroy() to a window beforeunload listener in the devtools
panel entry point. When the panel page is dismissed (devtools closed,
target tab closed, devtools navigated away) the chain now runs:
AIChat.destroy
-> AssistantController.destroy
-> PromptClient.destroy (disconnects the prompt-api port,
tells the background SW to destroy
the LanguageModel session)
-> AssistantTranscript.destroy (no-op)
Without this, the long-lived port and the model session stayed alive in
the background service worker until Chrome's SW idle GC took them out.
Wire AIChat.onTabActivated() to a click listener on the #ai-tab element. When the developer switches back to the AI Assistant tab, the transcript scrolls to the latest turn so they see the most recent reply without manually scrolling. The callback is deferred via requestAnimationFrame so the scroll runs after the TabBar click handler has flipped the selected attribute and the content is actually visible.
Five /**\n */ orphan blocks sat above functions and carried no content after the previous comment-stripping passes removed the descriptions inside them.
…seed clearConversation and setUrl destroy and recreate the LanguageModel session, but the panel's token counter, quota-exhausted styling, and input disabled state are only refreshed when AssistantController emits a capability-state change. Neither path was emitting one on the happy branch, so after clearing history the counter kept showing the pre-clear inputUsage — and if the user had cleared specifically to escape quota-exhausted, the input stayed disabled with 'Token quota exhausted. Clear history to continue.' Route both reseed paths through a new _reseedAndAnnounceReady helper that re-emits 'ready' after a successful reseed. The helper checks capability state before emitting so a failed reseed's session-failed banner is not immediately overwritten by ready.
clearConversation and setUrl destroy the active Prompt Client session synchronously and reseed asynchronously, while AIChat fires Clear History without awaiting the controller. A Send pressed in that window raced the Prompt Client's '\''No active session. Call createSession() first.'\'' guard. Track the in-flight reseed in a new _pendingReseed promise (set up synchronously at the destroy-then-reseed entry point so a Send issued mid-microtask still observes it). sendUserMessage awaits it before calling promptStreaming. If the reseed itself rejects, sendUserMessage rejects with the reseed error and the existing session-failed capability state stays on screen — we do not flip it to streaming-failed, which would overwrite the more accurate banner. Adds AssistantController spec coverage for the Clear History race, the URL-change race, and the failed-reseed case using a deterministic deferred fake createSession().
Inspection Context (the selected control's snapshot) now stays attached to every sendUserMessage until one of three explicit triggers detaches it: a different control is selected, the developer clicks the ✕ on the pill, or the inspected page navigates to a different URL. Clearing Conversation Memory no longer drops the snapshot — the two concepts are orthogonal. Controller changes: - Rename _pendingInspectionContext to _inspectionContext (the field is sticky, not pending). - sendUserMessage no longer nulls the snapshot after consuming it. - setUrl clears the snapshot on a real URL change (after dedupe) and emits a new inspection-context-cleared event before reseeding. - updateInspectionContext(null) emits inspection-context-cleared when a snapshot was attached; replacing one snapshot with another does not emit. - clearConversation does not touch the snapshot and does not emit inspection-context-cleared. View changes: - AIChat subscribes to inspection-context-cleared and hides the Context pill in the handler. - The ✕ click handler only calls updateInspectionContext(null); pill visibility flows through the event round-trip. Tests: - Existing #updateInspectionContext test is inverted — it now asserts the snapshot is injected into every subsequent prompt, not just the next one. - Adds regression coverage for clear-history + send, snapshot replacement, URL-change detach, dedupe path, never-persisted invariant, and the new event emission rules in both AssistantController and AIChat. PRD: .scratch/ai-inspection-context-sticky/PRD.md ADR: docs/adr/0002-inspection-context-lifecycle.md
…script
The chat transcript rendered "System"-labelled messages for two different
categories that never belonged there:
- Confirmations of a UI action the developer just performed
("Chat history cleared", "Context cleared - no control is currently
selected") — redundant with the visible state change (empty transcript,
disappearing pill).
- Transient errors for user actions ("Error: <stream failure>", "Error
clearing history: ...", "Failed to copy to clipboard") — category-confused;
errors are not conversation turns.
Both are gone. Confirmations are dropped outright: the empty transcript and
the hidden pill are themselves the confirmation. Errors move into a new
`.ai-error-slot` above the input row — a muted-red single-line status area
that stays hidden until an error occurs and auto-clears on the developer's
next input activity (input, keydown, or send).
The only surviving `appendSystemMessage` call is the 70%-usage advisory tip.
That is intentional — it is the only remaining message in that category that
genuinely belongs in the conversation flow.
AIChat changes:
- Add `.ai-error-slot` inside `.ai-input-area`, positioned between the
context-info pill and the input row. `role="status"` +
`aria-live="polite"` so screen readers announce errors non-interruptively.
- Add `_showError(message)` / `_clearError()` private helpers; `_showError`
replaces any current message (single-line, no stacking).
- Redirect three error paths (`stream-failed`, `_performClearHistory` catch,
clipboard-failure) into `_showError`.
- Drop the two confirmation `appendSystemMessage` calls from the
`conversation-cleared` handler and `_clearContext`.
- Wire `_clearError` into the input's `input`/`keydown` handlers and
`_handleSendMessage`.
AssistantTranscript changes:
- Accept an `onCopyFailed` callback via constructor options and invoke it
on clipboard failure instead of calling `appendSystemMessage`. The
transcript keeps its separation from the input area — it just notifies
the view, which owns the error slot.
- `appendSystemMessage` and the `_appendMessage('system', ...)` path remain
for the token-usage warning.
Tests:
- Invert prior `appendSystemMessage`-was-called assertions for the
clear-history, context-clear, stream-failed, and clipboard paths into
DOM/slot assertions (hidden state, text content, absence of a
`.message-system` node).
- Add coverage for slot placement, ARIA attributes, replace-not-stack
behaviour, and auto-clear on typing / keydown / send.
- Keep the token-usage warning assertion — no regression.
PRD: .scratch/ai-remove-system-messages/PRD.md
Issue: .scratch/ai-remove-system-messages/ISSUE-1-remove-system-messages.md
The .ai-error-slot rules were added to app/styles/less/modules/AIChat.less in db3fd0d but the compiled tests/styles/themes/{light,dark}/*.css were not regenerated in that commit. This runs grunt less to bring them back in sync.
The AI Chat token counter's default color (@gray-dark, #a3a3a3) failed WCAG AA against the panel background. Three severity classes already toggled by AIChat.js (warning, warning-critical, quota-exhausted) were either mis-colored or entirely missing in CSS. Changes: - .token-counter default: color @gray-dark -> @gray-darkest for legibility (#222 on light ~15:1, #949494 on dark ~5:1). - .warning: recolored from red to amber (@amber); font-weight normalized to 500 (was 600) so pill width stays constant across states. - .warning-critical: new rule, red (@red-saturated). Dark theme inherits the existing @red-saturated -> teal remap intentionally. - .quota-exhausted: new rule, dark red (@red-dark). - New theme variables @amber and @red-dark in light.less and dark.less with theme-appropriate hex values (per PRD). All four states use fade(<color>, 10%) for the pill background. No JS, DOM, ARIA, or threshold changes. No transitions or animations.
…ation
Match the AI Chat panel's post-clear state to its pristine fresh-load
state: when the conversation has no messages, neither the token-counter
pill nor the Clear History button appear in the input footer.
A single `_hasMessages` boolean on AIChat drives both. It flips true
on send, on stream-complete, and on non-empty conversation-loaded;
it flips false on conversation-cleared.
- Token counter starts with the semantic `hidden` attribute and
`.token-counter[hidden] { display: none; }` mirrors the existing
`.ai-error-slot[hidden]` idiom.
- `_updateTokenCounter()` returns early when `_hasMessages` is false,
clearing text and state classes and hiding the pill. When usage info
is briefly null or rejected in a non-empty conversation, the last
valid text and state class are retained rather than flickering off.
- Clear History button is hidden on conversation-cleared, revealed on
the first send, and its show path on the `ready` capability state is
gated on `_hasMessages` so a post-clear reseed's `ready` announcement
does not re-reveal it. The `session-failed` recovery path still
force-shows it.
Screen-reader semantics (role=status, aria-live=polite) unchanged.
Remove duplicate explanations of the _hasMessages gate across AIChat.js, consolidate Inspection Context stickiness docs onto updateInspectionContext, and shorten JSDoc on one-line delegates. Fix a stale comment on the streaming-failed capability config that referenced the removed system-message error path.
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.
Summary
Refactors the AI Assistant tab from a single 1500-line module into five small modules with explicit boundaries. Production behaviour is unchanged. The point is to make the code easier to test, easier to change, and easier to reason about.
Why
The AI tab mixed UI rendering, session lifecycle, conversation history, prompt construction, Chrome
prompt-apitransport, and streaming into one file. You couldn't test any of it without a real Chrome environment and a downloaded local model. This PR splits it into five units, each with a focused job and unit tests.What's in the PR
New modules
PromptBuilder— builds the system prompt, formats app metadata, formats the selected control as inspection context, truncates JSON, builds session seed messages. Pure functions. No Chrome APIs.PromptClient— single boundary around the backgroundprompt-apiport. Translates port-protocol status strings to canonical capability state names. Owns session lifecycle and streaming buffer.ConversationStore— persists conversation history per inspected URL. Wrapschrome.storage.localbehind a small async API.AssistantController— workflow coordinator. Owns capability state, conversation memory for the current URL, session creation, reseeding, per-turn context injection, streaming, and persistence. Has no DOM, Chrome, or storage dependency. Talks to the view through a local event bus.AssistantTranscript— renders the chat transcript. Owns the messages DOM, markdown, JSON expand/collapse, copy-to-clipboard, scrolling.Reshaped
AIChat— trimmed from ~1500 lines to ~560. Now a thin view over the controller. Owns input, banner, dialog, controller wiring. Delegates all state toAssistantControllerand all transcript rendering toAssistantTranscript.Removed
AISessionManager.js— replaced byPromptClient+AssistantController.ChatStorageManager.js— replaced byConversationStore.Boundaries
The view never talks to Chrome runtime, storage, or the model directly. Each arrow is a unit-testable seam.
Diff shape
Testing
grunt test— 436 tests passingkarma.conf.js)What's not in this PR
prompt-apiport contract.Status
The refactor is feature-complete on this branch. Left in draft until I get a first round of feedback from a maintainer.