Fix health-checks in the setting #1663
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRefactors STT connection hook to return { conn, local }; replaces banner components with structured health-check components for LLM and STT; removes monolithic segment utility and introduces a modular multi-pass segment pipeline and related passes; small UI and import updates across settings and hooks; local-stt server status mapping updated. Changes
Sequence Diagram(s)sequenceDiagram
actor UI
participant HealthCheck as HealthCheckForAvailability
participant useAvail as useAvailability
participant Config as ConfigStore
participant Providers as ProviderStore
UI->>HealthCheck: render
HealthCheck->>useAvail: call
useAvail->>Config: read current_provider,current_model
alt provider+model set
Config-->>useAvail: provider,model
useAvail->>Providers: read provider configs
Providers-->>useAvail: config
useAvail-->>HealthCheck: { available: true } or { available: false, message }
else missing provider/model
useAvail-->>HealthCheck: { available: false, message }
end
HealthCheck->>UI: render AvailabilityHealth if unavailable
sequenceDiagram
actor Caller
participant useSTTConn as useSTTConnection
participant RemoteCfg as RemoteConfig
participant LocalQuery as LocalQuery
Caller->>useSTTConn: call()
useSTTConn->>RemoteCfg: check configured provider/model
alt remote provider
RemoteCfg-->>useSTTConn: provider data
useSTTConn-->>Caller: { conn: connection, local: queryResult }
else local provider
useSTTConn->>LocalQuery: execute query
LocalQuery-->>useSTTConn: { data?: { connection } }
useSTTConn-->>Caller: { conn: local.data?.connection || null, local: queryResult }
end
Caller->>Caller: const { conn } = useSTTConnection()
sequenceDiagram
participant Client
participant Pipeline as buildSegments
participant Normalize as normalizeWordsPass
participant Resolve as resolveIdentitiesPass
participant Segmenter as segmentationPass
participant Propagate as identityPropagationPass
participant Merge as mergeSegmentsPass
participant Finalize as finalizeSegments
Client->>Pipeline: buildSegments(final, partial, hints)
Pipeline->>Normalize: run
Normalize-->>Pipeline: words
Pipeline->>Resolve: run
Resolve-->>Pipeline: frames
Pipeline->>Segmenter: run
Segmenter-->>Pipeline: proto segments
Pipeline->>Propagate: run
Propagate-->>Pipeline: segments
Pipeline->>Merge: run
Merge-->>Pipeline: merged segments
Pipeline->>Finalize: finalizeSegments
Finalize-->>Client: Segment[]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
apps/desktop/src/hooks/useSTTConnection.ts (1)
23-27: Refactored useSTTConnection hook looks sound and matches new consumersThe refactor to:
- Safely compute
isLocalModelwith!!current_stt_modelbeforestartsWith,- Wrap the local STT query result as
{ status, connection }and readlocal.data?.connectionin the memoizedconnection, and- Expose
{ conn: connection, local }as the public shape,all look consistent with downstream usage (
{ conn } = useSTTConnection()) and with the intent to surface both the effective connection and underlying local health/query state.One small optional tweak you might consider later: making
enabledmirrorisLocalModel(instead of onlycurrent_stt_provider === "hyprnote") to avoid running the no-op query when a Hyprnote provider is selected but a non-local model is chosen, though the current early-return in the queryFn already keeps it cheap.Also applies to: 29-33, 49-65, 71-78, 90-102
apps/desktop/src/utils/segment/segment.test.ts (1)
752-753: Consider dropping or gatingconsole.errorin tests
console.error(visualizeSegments(...))runs for every test case and can clutter test output. If you still want the visualization, consider guarding it behind a debug flag or switching toconsole.debug.apps/desktop/src/utils/segment/pass-merge-segments.ts (1)
3-58: Merge logic is sound; consider avoiding in‑place word mutationThe pass correctly:
- Short‑circuits when
segmentsis missing.- Only merges adjacent segments with identical keys when some speaker identity is present.
One minor thing:
mergeAdjacentSegmentsmutateslast.wordsin place. That’s fine given the current pipeline, but if you want passes to be strictly immutable, you could clone the segment/words when merging.apps/desktop/src/utils/segment/pass-normalize-words.ts (1)
3-43: Normalization is correct; consider small robustness tweaksThe normalization + ordering looks good overall. Two minor improvements you might consider:
When preserving
id,("id" in word && word.id ? … : {})will drop falsy ids (e.g.,""or0). If those are meaningful, using a null/undefined check instead would be safer:...("id" in word && word.id != null ? { id: String(word.id) } : {})Sorting only by
start_msdepends on the engine’s sort stability when timestamps are equal. If you want fully deterministic ordering, you could add a secondary key (e.g., original index) as a tie‑breaker.Neither is blocking, but they’d make the behavior a bit more predictable.
apps/desktop/src/utils/segment/pass-build-segments.ts (1)
12-138: Segmentation pass behavior matches the intended semantics; consider documenting invariantsThe combination of
segmentationPass,canExtendSegment, andplaceFrameInSegmentcorrectly implements:
- Segment keys derived from channel + optional speaker identity.
- Adjacent‑only merging for identity‑bearing segments.
- Careful rules for partial words and max gaps.
- Reuse of channel‑only segments for final frames without identity.
The logic around
canExtendSegment’s use ofsegments[segments.length - 1]and theactiveSegmentsmap is subtle but sound. A brief comment summarizing the invariants (e.g., “identity segments must be contiguous; channel‑only segments may interleave across channels”) would make this easier to maintain, but functionally this looks solid.apps/desktop/src/utils/segment/pass-resolve-speakers.ts (3)
10-68: Speaker identity resolution flow is clear and well-layeredThe stepwise enrichment (explicit assignment → speaker_index lookup → channel completion → lastSpeaker) is easy to follow and side-effect free, which is great.
One small nit: when both
speaker_indexandhuman_idare filled fromlastSpeaker,"last_speaker"is pushed twice intoprovenance. If downstream consumers conceptually treat provenance as a set, you might want to dedupe or push it once per source step instead of per-field update.
70-105: State updates look correct; consider whether channel mapping conditions match intentThe caching logic into
humanIdBySpeakerIndex,humanIdByChannel, andlastSpeakerByChannelis consistent with howresolveSpeakerIdentityreads them and avoids mutating theidentityobject in-place via spread, which is good.Two subtle behavior points you may want to double-check:
humanIdByChannelis only updated whenidentity.human_idis set andidentity.speaker_index === undefined(Lines 85-91). If, conceptually, a “complete” channel is tied to a stable human regardless of whether a provider speaker index is known, you might also want to populatehumanIdByChannelwhen both fields are present.lastSpeakerByChannelis not updated for final words whose identity is inferred solely viachannel_completionand has no explicit assignment orspeaker_index. This seems intentional (avoid heuristic propagation into finals), but it’d be good to confirm that it matches your streaming vs. finalized transcript semantics.
107-129: Identity resolution pass wiring looks solidThe pass correctly declares its dependency on
"words", maps each word to a resolved frame, and feeds back intospeakerStateviarememberIdentity, which makes the state evolution predictable.Given that
needs: ["words"]already encodes the requirement, defaultingwordsto[]withgraph.words ?? []is slightly forgiving: if someone runs this pass outside your guarded pipeline, a missingwordsfield will silently yield an emptyframesarray instead of surfacing an error. If you want stricter behavior in that scenario, you could throw whengraph.wordsisundefinedrather than defaulting.apps/desktop/src/utils/segment/index.ts (2)
33-62: Segment pipeline orchestration is clean and defensive
buildSegmentsplusdefaultSegmentPassesgive a clear, linear pipeline: normalize → resolve speakers → build segments → propagate → merge. Early return when both word arrays are empty avoids unnecessary work, and usingensurePassRequirementsbefore each pass is a nice guardrail against ordering mistakes.If you anticipate callers wanting to run custom pass sequences or tap into intermediate graphs (e.g., for debugging/health checks), consider exporting
runSegmentPipelineandcreateSegmentPassContextas public helpers rather than keeping them internal, but that’s purely an API design choice.
64-100: Speaker state initialization makes sense; hard-coded channels may limit future flexibilityThe use of maps and sets for
assignmentByWordIndex,humanIdBySpeakerIndex,humanIdByChannel, andlastSpeakerByChannelis appropriate, and merging multiple hints perwordIndexvia thecurrentobject is a nice touch.Two design considerations:
completeChannelsis currently seeded with0unconditionally and1whennumSpeakers === 2(Lines 73-77). IfChannelProfileever becomes non-numeric or you support more than two channels, this hard-coding could become a limitation; you might eventually want to derive “complete” channels from options or input metadata instead of fixed indices.humanIdBySpeakerIndexis only populated from hints here. If you later infer new(speaker_index, human_id)pairs during resolution, it could be worth ensuring those also flow back into this map (e.g., viarememberIdentity)—which you already do—so the map stays authoritative over time.Nothing blocking, just worth keeping in mind as you evolve the model.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
apps/desktop/src/components/main/body/sessions/shared.tsx(1 hunks)apps/desktop/src/components/settings/ai/llm/banner.tsx(0 hunks)apps/desktop/src/components/settings/ai/llm/health.tsx(1 hunks)apps/desktop/src/components/settings/ai/llm/index.tsx(1 hunks)apps/desktop/src/components/settings/ai/llm/select.tsx(2 hunks)apps/desktop/src/components/settings/ai/shared/health.tsx(1 hunks)apps/desktop/src/components/settings/ai/shared/index.tsx(0 hunks)apps/desktop/src/components/settings/ai/shared/list-common.ts(1 hunks)apps/desktop/src/components/settings/ai/stt/health.tsx(3 hunks)apps/desktop/src/components/settings/ai/stt/index.tsx(1 hunks)apps/desktop/src/components/settings/ai/stt/select.tsx(2 hunks)apps/desktop/src/hooks/useRunBatch.ts(1 hunks)apps/desktop/src/hooks/useSTTConnection.ts(4 hunks)apps/desktop/src/hooks/useStartListening.ts(1 hunks)apps/desktop/src/utils/index.ts(1 hunks)apps/desktop/src/utils/segment.ts(0 hunks)apps/desktop/src/utils/segment/index.ts(1 hunks)apps/desktop/src/utils/segment/pass-build-segments.ts(1 hunks)apps/desktop/src/utils/segment/pass-merge-segments.ts(1 hunks)apps/desktop/src/utils/segment/pass-normalize-words.ts(1 hunks)apps/desktop/src/utils/segment/pass-propagate-identity.ts(1 hunks)apps/desktop/src/utils/segment/pass-resolve-speakers.ts(1 hunks)apps/desktop/src/utils/segment/segment.test.ts(1 hunks)apps/desktop/src/utils/segment/shared.ts(1 hunks)plugins/local-stt/src/server/external.rs(1 hunks)
💤 Files with no reviewable changes (3)
- apps/desktop/src/components/settings/ai/shared/index.tsx
- apps/desktop/src/components/settings/ai/llm/banner.tsx
- apps/desktop/src/utils/segment.ts
🧰 Additional context used
🧬 Code graph analysis (17)
apps/desktop/src/utils/segment/pass-propagate-identity.ts (1)
apps/desktop/src/utils/segment/shared.ts (3)
ProtoSegment(92-95)SpeakerState(117-123)SegmentPass(105-109)
apps/desktop/src/utils/segment/pass-build-segments.ts (1)
apps/desktop/src/utils/segment/shared.ts (7)
SegmentPass(105-109)ProtoSegment(92-95)SpeakerIdentity(68-71)SegmentKey(41-45)SegmentKey(47-54)ResolvedWordFrame(81-85)SegmentBuilderOptions(56-59)
apps/desktop/src/components/settings/ai/shared/health.tsx (1)
packages/utils/src/cn.ts (1)
cn(20-22)
apps/desktop/src/utils/segment/pass-resolve-speakers.ts (1)
apps/desktop/src/utils/segment/shared.ts (6)
SegmentWord(20-20)SpeakerIdentity(68-71)SpeakerState(117-123)SpeakerIdentityResolution(87-90)IdentityProvenance(73-77)SegmentPass(105-109)
apps/desktop/src/utils/segment/pass-merge-segments.ts (1)
apps/desktop/src/utils/segment/shared.ts (4)
SegmentPass(105-109)SegmentKey(41-45)SegmentKey(47-54)ProtoSegment(92-95)
apps/desktop/src/components/settings/ai/llm/select.tsx (1)
apps/desktop/src/components/settings/ai/llm/health.tsx (1)
HealthCheckForConnection(11-40)
apps/desktop/src/components/main/body/sessions/shared.tsx (1)
apps/desktop/src/hooks/useSTTConnection.ts (1)
useSTTConnection(9-103)
apps/desktop/src/utils/segment/index.ts (6)
apps/desktop/src/utils/segment/shared.ts (11)
WordLike(11-16)RuntimeSpeakerHint(31-34)SegmentBuilderOptions(56-59)Segment(36-39)SegmentGraph(97-103)SegmentPass(105-109)SpeakerState(117-123)SpeakerIdentity(68-71)SegmentPassContext(111-115)ProtoSegment(92-95)SegmentWord(20-20)apps/desktop/src/utils/segment/pass-normalize-words.ts (1)
normalizeWordsPass(33-43)apps/desktop/src/utils/segment/pass-resolve-speakers.ts (1)
resolveIdentitiesPass(107-130)apps/desktop/src/utils/segment/pass-build-segments.ts (1)
segmentationPass(12-30)apps/desktop/src/utils/segment/pass-propagate-identity.ts (1)
identityPropagationPass(45-61)apps/desktop/src/utils/segment/pass-merge-segments.ts (1)
mergeSegmentsPass(3-13)
apps/desktop/src/utils/segment/shared.ts (1)
apps/desktop/src/utils/segment/index.ts (9)
ChannelProfile(22-22)ChannelProfileSchema(23-23)WordLike(30-30)PartialWord(25-25)SegmentWord(29-29)RuntimeSpeakerHint(26-26)Segment(27-27)SegmentKey(24-24)SegmentBuilderOptions(28-28)
apps/desktop/src/components/settings/ai/llm/index.tsx (1)
apps/desktop/src/components/settings/ai/llm/health.tsx (1)
HealthCheckForAvailability(73-81)
apps/desktop/src/utils/segment/pass-normalize-words.ts (1)
apps/desktop/src/utils/segment/shared.ts (3)
WordLike(11-16)SegmentWord(20-20)SegmentPass(105-109)
apps/desktop/src/hooks/useStartListening.ts (1)
apps/desktop/src/hooks/useSTTConnection.ts (1)
useSTTConnection(9-103)
apps/desktop/src/hooks/useRunBatch.ts (1)
apps/desktop/src/hooks/useSTTConnection.ts (1)
useSTTConnection(9-103)
apps/desktop/src/components/settings/ai/stt/index.tsx (1)
apps/desktop/src/components/settings/ai/stt/health.tsx (1)
HealthCheckForAvailability(37-45)
apps/desktop/src/components/settings/ai/stt/select.tsx (2)
apps/desktop/src/components/settings/ai/llm/health.tsx (1)
HealthCheckForConnection(11-40)apps/desktop/src/components/settings/ai/stt/health.tsx (1)
HealthCheckForConnection(9-12)
apps/desktop/src/components/settings/ai/llm/health.tsx (4)
apps/desktop/src/components/settings/ai/stt/health.tsx (2)
HealthCheckForConnection(9-12)HealthCheckForAvailability(37-45)apps/desktop/src/components/settings/ai/shared/health.tsx (2)
ConnectionHealth(14-50)AvailabilityHealth(52-66)apps/desktop/src/hooks/useLLMConnection.ts (1)
useLanguageModel(21-94)apps/desktop/src/config/use-config.ts (1)
useConfigValues(42-72)
apps/desktop/src/components/settings/ai/stt/health.tsx (3)
apps/desktop/src/components/settings/ai/llm/health.tsx (1)
HealthCheckForConnection(11-40)apps/desktop/src/components/settings/ai/shared/health.tsx (2)
ConnectionHealth(14-50)AvailabilityHealth(52-66)apps/desktop/src/hooks/useSTTConnection.ts (1)
useSTTConnection(9-103)
🪛 Biome (2.1.2)
apps/desktop/src/components/settings/ai/llm/health.tsx
[error] 55-55: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ci (macos, macos-14)
- GitHub Check: fmt
🔇 Additional comments (15)
apps/desktop/src/components/settings/ai/shared/list-common.ts (1)
21-29: LGTM! Good addition for completeness.Adding "sora" to the ignore keywords is consistent with filtering out non-LLM models (video generation) alongside other specialized models like embeddings, TTS, and image generation. This aligns well with the PR's goal of refining model classification for health-check workflows.
apps/desktop/src/components/settings/ai/stt/index.tsx (1)
2-2: LGTM!Clean replacement of the banner component with the health-check approach. The new
HealthCheckForAvailabilitycomponent provides structured availability feedback.Also applies to: 8-8
apps/desktop/src/components/settings/ai/llm/select.tsx (1)
26-26: LGTM!The conditional rendering of
HealthCheckForConnectionis well-gated, ensuring the health check only displays when both provider and model are configured. This aligns with the broader health-check refactor across the AI settings.Also applies to: 150-152
apps/desktop/src/components/settings/ai/llm/index.tsx (1)
2-2: LGTM!Consistent with the STT health-check refactor. The replacement of
BannerForLLMwithHealthCheckForAvailabilityprovides a more structured approach to availability feedback.Also applies to: 8-8
apps/desktop/src/components/settings/ai/stt/select.tsx (1)
16-16: LGTM!The conditional rendering of
HealthCheckForConnectionmirrors the pattern in LLM select.tsx, maintaining consistency across the AI settings.Also applies to: 182-184
apps/desktop/src/components/settings/ai/shared/health.tsx (1)
1-66: LGTM!Well-designed shared health components with a clean discriminated union pattern for
ConnectionHealth. The components provide clear visual feedback for connection status and availability issues.apps/desktop/src/components/settings/ai/llm/health.tsx (1)
11-40: LGTM!The
HealthCheckForConnectioncomponent properly maps connection states to UI props, and theHealthCheckForAvailabilitycomponent with itsuseAvailabilityhook provides clear availability messaging. The memoization inuseAvailabilityis appropriate given the dependency on configuration state.Also applies to: 73-121
apps/desktop/src/components/main/body/sessions/shared.tsx (1)
68-68: STT connection destructuring correctly aligned with new hook APIUsing
const { conn: sttConnection } = useSTTConnection();matches the updated{ conn, local }return shape and keepsisDisabled/warningMessagesemantics identical to the previous truthiness check on the connection. Looks good.apps/desktop/src/hooks/useStartListening.ts (1)
18-18: useStartListening correctly adapted to new STT connection shapeDestructuring
{ conn }fromuseSTTConnection()is consistent with the new{ conn, local }API, and the existingif (!conn || !store)guard still protects all usages ofconn. Dependency array already includesconn, so the hook remains correct.apps/desktop/src/hooks/useRunBatch.ts (1)
37-37: useRunBatch STT connection wiring remains correct after refactorSwitching to
const { conn } = useSTTConnection();cleanly follows the new hook return shape. The early!connguard, provider selection, and batch params construction all continue to work as before, andconnis included in the callback dependencies.apps/desktop/src/utils/segment/shared.ts (1)
3-123: Segment/shared type modeling looks consistent and future‑proofThe enum, word/segment types,
SegmentKeyfactory, andSegmentGraph/SegmentPasswiring line up cleanly with how the passes and tests use them (channels as a numeric enum, optional speaker identity, staged graph fields). I don’t see correctness or ergonomics issues here; this is a solid foundation for the pipeline.apps/desktop/src/utils/index.ts (1)
1-3: Barrel re‑export for segment utilities is appropriateRe‑exporting
./segmentfrom the utils index keeps the public surface coherent and doesn’t introduce any obvious coupling issues.apps/desktop/src/utils/segment/segment.test.ts (1)
3-3: Updated segment test import is correctSwitching to the module index (
".") and importingWordLikeas a type matches the new segment barrel structure and avoids pulling in unnecessary runtime values.apps/desktop/src/utils/segment/pass-propagate-identity.ts (1)
9-61: Channel‑level identity propagation is implemented cleanlyThe pass:
- Preserves immutability at the graph level by cloning segments/word arrays.
- Only stamps
speaker_human_idonto segments for completed channels that don’t already have one, while preserving any existingspeaker_index.This aligns well with the intended “complete channel identity” behavior; no changes needed.
apps/desktop/src/utils/segment/index.ts (1)
102-146: Context creation and pipeline helpers are robust and easy to reason about
createSegmentPassContextclonesoptionsso passes can safely mutate their view without surprising callers.ensurePassRequirementsis a simple, effective guard that fails fast when a pass’sneedsaren’t satisfied.runSegmentPipelinecomposes passes functionally, andfinalizeSegmentscleanly strips the internalorderfield while preserving the rest of the word shape asSegmentWord.All of this looks solid and idiomatic; no issues from my side.
No description provided.