Review: cumulative changes since /ask slice (f15e8ba..9d3d275)#1
Closed
birdnamoo wants to merge 36 commits into
Closed
Review: cumulative changes since /ask slice (f15e8ba..9d3d275)#1birdnamoo wants to merge 36 commits into
birdnamoo wants to merge 36 commits into
Conversation
… not just bubble count
poll_health returned as soon as embedderReady became true, freezing the cached generatorReady at false forever. The generator loads lazily (first /chat or /prepare-generation), long after embedderReady, so host_status reported generatorReady:false even when the host /health said true. The frontend chat/ask gate keys on host_status.generatorReady, so it never advanced past "preparing" and local AI Q&A hung indefinitely through the UI. Extract the per-poll decision into a pure poll_action() (unit-tested) and keep the loop alive for the host lifetime, refreshing generatorReady on every poll. Once Ready it never gives up (a healthy host is not marked Unavailable by the startup ceiling); the my_gen guard still lets a respawn or shutdown stop a stale thread.
The prerequisite polled host_status for generatorReady but sent no question, so the lazily-loaded generator was never triggered and the gate timed out even on a healthy host. Kick off prepare_generation (idempotent) once the host is Ready so the gate can observe generatorReady.
Keeping the 1s poll for the host lifetime floods host-out.log: Kestrel logs ~2 lines per /health request, so steady-state polling wrote ~2 lines/second indefinitely. After Ready the only thing left to observe is the lazily-loaded generator flipping generatorReady, which needs no sub-second latency — back the interval off to 10s. Startup detection keeps the 1s cadence.
… optimistically on prepare
… harden /health test; type=button on Retry
A failed generation leaves an empty or half-finished assistant turn at the tail of the conversation: run() pushes the assistant turn before streaming, and an error mid-stream (or at the gate) flips status to 'error' without removing it. retryGeneration() dropped that orphan, but the composer and Enter stay enabled in the 'error' state (busy only covers searching/ generating), so a user who types a new question instead of clicking Retry hit send() -- which appended a user turn without dropping the orphan. run() then forwarded [user1, assistant_orphan, user2] to buildChatMessages, which maps every turn verbatim, so the model received an empty/partial assistant reply as if it had already answered. Factor the drop into a pure, status-gated pruneOrphanedAssistantTurn helper (only prunes in the 'error' state, so a finalized assistant turn from a healthy run is preserved) and call it from both send() and retryGeneration().
v0.2.0 is the acquisition-launch release: it ships the full free local AI wedge (semantic search, related notes, grounded Q&A, multi-turn chat) and bundles the publishing sidecar so in-app "Publish site" now works (resolving the v0.1.1 packaged-build limitation). cargo metadata --locked: exit 0.
tauri.conf.json bundle.resources requires both resources/canopy and resources/host to exist for any cargo build of the app. The host sidecar was assembled after the "Verify bundled sidecar publishes" step, so that step's cargo build failed on a missing resources/host path -- first surfaced on the v0.2.0 tag, the first release since host bundling was added. Move .NET setup and host assembly ahead of both verify steps so both resource dirs are staged before any cargo build.
The cargo test step builds the Tauri crate, whose tauri.conf.json bundle.resources requires both resources/canopy and resources/host to exist. Without assembling them the build script failed with "resource path doesn't exist", leaving the Rust job permanently red on CI (local passed only because the working tree had assembled resources). Add the same canopy + host assembly steps release.yml uses, before cargo test. Canopy ref is pinned in sync with release.yml.
The two banner buttons had no styles, so they rendered as default OS buttons (white fill, dark border) that clashed with the dark theme. Integrate them into the app's established button hierarchy: 'Update & restart' becomes the accent-fill primary (matching .chat-send / .chat-enable) and 'Later' a quiet ghost that yields to it. Add a small emerald cue dot for update-ready status, trailing-align the actions, and cover focus-visible / disabled / reduced-motion. All tokens, no hardcoded values.
Typing then pausing triggers the debounced auto-save; the editor then lost focus, breaking continuous typing. Root cause: an atomic write (temp in .textree/tmp -> rename onto the note) surfaces as MULTIPLE OS events on the same final path (Remove+Create+Modify on Windows, confirmed by instrumenting the real watcher). The watcher processed each event separately and the self-write registry is consume-once, so the trailing events leaked past suppression as false external changes -> sync reloadActive -> reloadVersion bump -> Editor recreation -> lost focus. Fix: collapse each debounce flush to one change per path by final filesystem state (exists -> one self-write-checked Modified; gone -> Removed), via new process_batch + changes_from_events. The self-write registry is consulted once per path, so a self-write can no longer leak. Tests: process_batch unit cases (multi-event self-write -> zero changes; external edit/removal/distinct paths) + a real-OS integration test (#[ignore]) asserting a real atomic_write emits zero external changes (3x stable).
…tion) Symptom-level regression for the focus-loss fix (ba8eeb9): type, wait past the save + watcher debounce, then keep typing WITHOUT re-focusing. Pre-fix the watcher echo recreated the editor and dropped focus, so the second burst never reached disk; this proves the symptom (focus), complementing the watcher unit/ integration tests that prove the mechanism. Verified 4/4 green on the dev app.
Patch release: editor focus-loss fix (watcher self-write echo), update-banner styling, ci.yml Rust job fix.
…, drop stale comment
The app is built with `windows_subsystem = "windows"`, which only suppresses a console for the app process itself. Console-subsystem children we spawn — the .NET host sidecar, the canopy Node CLI, and taskkill — still got their own console window on Windows because no `CREATE_NO_WINDOW` flag was set. The user saw a console window flash on app launch/update (the host sidecar). Add `process_ext::no_console_window`, a small cross-platform helper that sets `CREATE_NO_WINDOW` on Windows and is a no-op elsewhere, and apply it at all three spawn sites. Centralizing the flag keeps the magic constant in one place. cargo test: 113 passed, 0 failed (MSVC). Console suppression itself is a Windows GUI runtime behavior and is verified by running the built app, not unit tests.
…a-label + clipboard catch
…t spawn, enlarge mode toggle Quality/defect cleanup across the workspace-chat surface (no new IPC, no new deps): - chatStore.cancel() now drops the in-flight (half-streamed) assistant turn it was leaving frozen in the session — switching Note<->Chat or closing the app mid-generation resurfaced a truncated answer with clickable partial citations under a silent 'idle' status. Generalize the pure pruneOrphanedAssistantTurn helper to treat both 'error' and 'generating' as orphan-leaving states. - ChatView: move aria-live off the whole .chat-turns container (which re-announced the entire conversation on every streamed token) onto the streaming assistant bubble only; add a visually-hidden speaker prefix (You/Assistant) for screen readers (WCAG 4.1.3, 1.3.1). - host.rs: spawn_host's idempotent guard was a check-without-set TOCTOU — two concurrent callers (mount auto-spawn racing the ?-enable) could both pass and double-spawn, orphaning the first child and leaking its port. Add an atomic try_begin_spawn that claims Starting under a single status lock. - Note|Chat toggle: raise .mode-btn to a 24px min target (WCAG 2.5.8) and drop a hardcoded 2px padding in favour of spacing tokens. ARIA was already correct (role=group + aria-pressed). Tests: vitest 235 (+2), cargo 116 (+3, incl. a 16-thread single-winner race test). check 0/0, build ok.
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
2 participants
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.
Throwaway PR for an external cumulative code review (
/code-review ultra). Not for merge — will be closed after review.Scope
f15e8ba..9d3d275— 36 commits, 32 files, +2,543 / −954.Cumulative surface merged to
mainand shipped in/after v0.2.0 that has not yet had an external cloud review pass:0f5bdfd~)generatorReadypropagation (chat/ask never reaching/chat)76767f1)Note
All commits are already on public
main. Base/head are temporaryreview/*branches created only to express the comparison range for the review.