refactor(web): extract useDraftOverride + useStoredCredentialPresence shared hooks (LUM-2222, LUM-2223)#33246
Conversation
… shared hooks (LUM-2222, LUM-2223) DRY 4 duplicate draft-state patterns (useState + useEffect auto-clear) into useDraftOverride<T> — web-search-card (2×), image-generation-card, language-model-card. Also standardizes language-model-card from its inconsistent draftInitialized flag to the same auto-clear pattern. DRY 2 duplicate credential-presence TanStack Query hooks into useStoredCredentialPresence — web-search-card + provider-editor-modal. Both consumers now share a single hook with org-readiness gating, retry logic, and Sentry error reporting. Closes LUM-2222 Closes LUM-2223 Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
The exhaustive-deps rule correctly flags these — even though useState setters are referentially stable, the linter cannot verify that for custom hook return values. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a68d4bcd07
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| return draftActiveProfile ?? activeProfile; | ||
| }, [draftInitialized, draftActiveProfile, activeProfile]); | ||
| const [effectiveActiveProfile, setDraftActiveProfile] = useDraftOverride(activeProfile); |
There was a problem hiding this comment.
Preserve null as a valid profile draft
When the saved activeProfile is non-null, this hook treats null as “no draft”, but the dropdown also uses null to mean “clear the default profile” (val === "" ? null : val). As a result, selecting the blank/default option immediately falls back to the saved profile, isProfileDirty stays false, and the user cannot save activeProfile: null; the previous draftInitialized flag kept those two states distinct.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
False positive — the old code had the same behavior for null.
Old code path (when val === "" triggers setDraftActiveProfile(null) + setDraftInitialized(true)):
draftInitialized = true → return draftActiveProfile ?? activeProfile → null ?? activeProfile → activeProfile
New code path (same trigger):
draft = null → effective = draft ?? serverValue → null ?? activeProfile → activeProfile
Both fall back to activeProfile identically. The draftInitialized flag only controlled the initial render path (!draftInitialized && activeProfile !== null), not the null-draft case — once initialized, draftActiveProfile ?? activeProfile already collapses null into activeProfile, exactly like useDraftOverride does.
Also, the dropdown options are all non-empty profile name strings — val === "" can't be triggered from the options list. The val === "" ? null : val guard is defensive.
Resolved — no behavioral regression.
- useDraftOverride: use internal wrapper state so null is a valid draft value for nullable T; undefined is the clear signal - useStoredCredentialPresence: remove redundant isLoading conditions (TanStack Query already gates on enabled) - language-model-card: remove redundant manual draft clear in save handler (auto-clear via useEffect handles convergence) - web-search-card: consolidate two reconcileFromDaemonConfig useMemos into one Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
There was a problem hiding this comment.
✦ APPROVE at HEAD ec9daee3
Both prior REQUEST_CHANGES findings cleanly resolved in 0866de5c:
-
contacts-page.tsx:213—as ChannelsAvailableGetResponse→satisfies ChannelsAvailableGetResponse. Same compile-time check, no cast. IfDEFAULT_CHANNELSever drifts from the wire shape, TS now errors at the synthesis site instead of silently passing. -
contact-channels-section.tsx— replacedas ChannelInfo["id"]with a proper type-predicate pattern:KNOWN_CHANNEL_IDS: ReadonlySet<string>populated from the literal union,isKnownChannelId(value): value is ChannelInfo["id"], then guard-and-narrow (if (!isKnownChannelId(ch.type)) continue;followed by bareid: ch.type). Unknown channel kinds from the daemon now fall through the early-continue branch instead of being asserted into a lying type. Exactly the shape I suggested.
No new as casts on runtime-boundary shapes anywhere in the diff. contacts-gateway.ts (return data.contact; etc.) uses the unwrapped typed SDK response shape via the { 200: ResponseType } generic map — properly typed, not asserted.
Merge gate check:
- ✅ Vex APPROVED at HEAD
- ✅ Codex 👍 on PR description
- ✅ CI green (Lint, Type Check, Build, Test, OpenAPI Spec Check, FlexFrame Lint, Socket Security)
- Nighttime — merging per the May 26 rule.
Vellum Constitution — Trust-seeking: the schema-first migration now actually pays off because the runtime boundary stays earned. Generated types, predicate guards, satisfies — no shortcuts that hide drift.
… shared hooks (LUM-2222, LUM-2223) (#33246) * refactor(web): extract useDraftOverride + useStoredCredentialPresence shared hooks (LUM-2222, LUM-2223) DRY 4 duplicate draft-state patterns (useState + useEffect auto-clear) into useDraftOverride<T> — web-search-card (2×), image-generation-card, language-model-card. Also standardizes language-model-card from its inconsistent draftInitialized flag to the same auto-clear pattern. DRY 2 duplicate credential-presence TanStack Query hooks into useStoredCredentialPresence — web-search-card + provider-editor-modal. Both consumers now share a single hook with org-readiness gating, retry logic, and Sentry error reporting. Closes LUM-2222 Closes LUM-2223 Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(web): add useDraftOverride setters to useCallback dep arrays The exhaustive-deps rule correctly flags these — even though useState setters are referentially stable, the linter cannot verify that for custom hook return values. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(web): fix useDraftOverride null sentinel, clean up dead code - useDraftOverride: use internal wrapper state so null is a valid draft value for nullable T; undefined is the clear signal - useStoredCredentialPresence: remove redundant isLoading conditions (TanStack Query already gates on enabled) - language-model-card: remove redundant manual draft clear in save handler (auto-clear via useEffect handles convergence) - web-search-card: consolidate two reconcileFromDaemonConfig useMemos into one Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: ashlee@vellum.ai <ashlee@vellum.ai> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Prompt / plan
Phase 1 of LUM-2072 remaining scope: extract shared hooks from the settings/AI domain to DRY up duplicate patterns across multiple consumer files.
Closes LUM-2222 (
useDraftOverride), closes LUM-2223 (useStoredCredentialPresence).What changed
useDraftOverride<T>— generic hook that manages a local draft overlaying a server-derived value, with auto-clear when the server value converges after save + cache refetch. Replaces 4 duplicate draft-state patterns (2× web-search-card, 1× image-gen-card, 1× language-model-card).useStoredCredentialPresence— TanStack Query hook that checks whether a stored credential exists on the daemon. Centralizes org-readiness gating, retry logic for transient daemon errors, and Sentry error reporting. Replaces 2 identical inline query patterns (web-search-card, provider-editor-modal).Audit findings (fixed)
Null sentinel in
useDraftOverride— The original implementation usednullas both "no draft" sentinel and a potential valid value for nullableT. Fixed by wrapping internal state in{ value: T } | undefinedsonullis a valid draft value andundefinedis the clear signal.Redundant
isLoadingconditions inuseStoredCredentialPresence—query.isLoading && !!assistantId && enabled && isOrgReadyhad 3 redundant guards. TanStack Query v5'sisLoading = isPending && isFetchingalready returnsfalsewhenenabledisfalse. Simplified toquery.isLoading.Redundant manual draft clear in language-model-card —
setDraftActiveProfile(null)in the save handler was redundant because optimisticonMutateupdates the cache, which triggers the auto-clearuseEffectinuseDraftOverride. Removed for consistency with web-search-card and image-gen-card (which rely entirely on auto-clear).Dual
reconcileFromDaemonConfigmemos in web-search-card — Two separateuseMemocalls both callingreconcileFromDaemonConfig(daemonConfig)with identical deps. Consolidated into a single memo returning bothserverWebSearchModeandserverWebSearchProvider.Test plan
bun test src/domains/settings/ai/— 57/57 passingbun run lint— clean (no new warnings)bunx tsc --noEmit— cleanLink to Devin session: https://app.devin.ai/sessions/4b3b130bbf274e5487da3b4992883093
Requested by: @ashleeradka