fix(dashboard): persist model param-filter edits on popover close (#8910) - #9013
Merged
diegosouzapw merged 10 commits intoAug 13, 2026
Conversation
…uzapw#8910) ModelCompatPopover declared providerId/modelId in its props type but never destructured them, so both param-filter fetches referenced undefined identifiers (TS2304, frozen in the dashboard-typecheck baseline) and threw into a silent catch. CustomModelsSection also never passed the two props. - Destructure providerId/modelId; pass them from CustomModelsSection. - Save pending block/allow drafts when the popover closes or unmounts, so an outside mousedown no longer discards them. - Read drafts from refs at save time and guard concurrent saves, avoiding stale-closure payloads and duplicate PUTs. - Keep dirty state and drafts on non-OK/failed GET or PUT instead of silently clearing them; skip state updates after unmount. - Provider-level block/allow, autoLearn, and other model entries are preserved; an empty block+allow still removes only the selected model entry. - Ratchet the three now-clean dashboard-typecheck baseline entries. Compat-toggle and upstream-header paths are unchanged.
…es (diegosouzapw#8910) The close-time save could clear the dirty flag for a payload snapshotted before the PUT resolved, silently discarding any keystroke that landed in that window. Track a monotonic draft revision and only acknowledge the revision that was actually written, re-running the save (bounded) otherwise. A failed save previously stayed dirty to 'retry on a later close', but reopening the popover reloaded server state and silently reverted the draft. Keep a dirty draft for the same provider/model on reopen and show a failure marker next to the saving indicator instead.
…obber (diegosouzapw#8910) The retained-draft guard in the param-filters load effect required paramLoadedKeyRef to match the current target, but that ref was only assigned after a successful GET. Any draft typed before a successful load for that target was therefore unguarded, and the clean-slate write overwrote both the text and the dirty flag: - a draft typed while the INITIAL load GET was still in flight was overwritten and its dirty flag cleared, so the close-path save became a no-op and the keystrokes vanished with no feedback; - after a FAILED initial load, the retained draft was destroyed by the next successful reopen load — the exact moment the user reopens to retry — and the failure indicator was cleared as if the save had succeeded. Track the target on the dirty flag itself (paramDirtyKeyRef, set when the draft is marked dirty) instead of deriving it from a completed load, and re-check the guard after the GET await so a load result never overwrites text, clears dirty, or clears the failure indicator for a draft that is not on the server.
…iegosouzapw#8910) saveModelParamFilters guarded on paramDirtyRef alone and read the providerId/modelId it closed over, never the target the draft was typed for. ModelCompatPopover is not always keyed by a stable identity (CompatibleModelsSection keys by `${alias}:${modelId}`, PassthroughModelsSection by the full model string, and providerId is threaded from route/page state), so a re-render can re-point a live, mounted popover at a different provider/model. If the old target's save had failed or never ran, the still-dirty draft was then PUT into the NEW target — writing a filter list under a model/provider the user never edited and destroying that target's real config. Replace the dirty flag / revision counter / dirty-key trio with a single ParamFilterDraft ref that carries the provider, model and both field values captured at edit time. The save drives its GET, PUT and payload from that draft instead of the current props, re-reads the ref after each await (restarting the attempt if the draft was replaced by one for another target), and only clears it when the exact draft object it wrote is still pending. Object identity replaces the revision counter, keeping the existing lost-update protection. A load no longer clears the draft or the failure indicator: a draft pending here belongs to another target and is still owed a write to it. An orphaned draft is therefore neither dropped nor redirected — it keeps its own provider/model, keeps the failure marker visible, and is retried by the next blur/close/unmount save. The cleanup effect also depends on the target key so re-pointing the popover flushes the old draft.
…n target (diegosouzapw#8910) Two remaining defects of the diegosouzapw#8910 silent-data-loss family, both reached through the re-point path of a live ModelCompatPopover. 1. The inputs render blockText/allowText, whose only writer was the load effect — and that effect early-returned whenever a draft was dirty for the target. So re-pointing A -> B -> A left B's server values on screen under A, and the next keystroke snapshotted them into A's draft, persisting B's content into A's entry. The fields are now a function of the target: on return to a target with a pending draft the draft is restored into the inputs, and on a target with no draft the previous target's values are cleared instead of being left behind. An edit also no longer trusts the counterpart field unless the values on screen belong to the target being edited. 2. The pending draft lived in a single slot that every edit overwrote, so typing into a newly pointed target destroyed the previous target's unsaved work while the new target's successful save cleared the failure indicator — a green UI over data that was never written. Drafts are now keyed by provider/model; the save drains every pending draft against its own target, and the indicator reflects unsaved work across all targets rather than the last write. Regression tests: modelCompatPopover-param-filter-target-repoint.test.tsx (3 cases, RED at ecd1114, GREEN here). Scope limited to this component.
Owner
Review: PR #9013 — fix(dashboard): persist model param-filter edits on popover closeVerdict: fix-in-place (4/5 stars) High-quality fix with a draft-based architecture and excellent test coverage (7 test files, ~832 lines). Pre-merge (minor)
Ready for merge after those tweaks. |
diegosouzapw
pushed a commit
to Egorich-print/OmniRoute-Rust
that referenced
this pull request
Aug 11, 2026
diegosouzapw
marked this pull request as ready for review
August 13, 2026 10:54
diegosouzapw
merged commit Aug 13, 2026
4ff9f0d
into
diegosouzapw:release/v3.8.50
12 of 13 checks passed
Owner
|
Merged into |
tkgo11
pushed a commit
to tkgo11/OmniRoute
that referenced
this pull request
Sep 23, 2026
muhamadgalihsaputra
pushed a commit
to niyatna/NiyatnaRoute
that referenced
this pull request
Sep 27, 2026
…egosouzapw#8910) (diegosouzapw#9013) * test: reproduce model param filter close persistence * fix(dashboard): persist model param filters on popover close (diegosouzapw#8910) ModelCompatPopover declared providerId/modelId in its props type but never destructured them, so both param-filter fetches referenced undefined identifiers (TS2304, frozen in the dashboard-typecheck baseline) and threw into a silent catch. CustomModelsSection also never passed the two props. - Destructure providerId/modelId; pass them from CustomModelsSection. - Save pending block/allow drafts when the popover closes or unmounts, so an outside mousedown no longer discards them. - Read drafts from refs at save time and guard concurrent saves, avoiding stale-closure payloads and duplicate PUTs. - Keep dirty state and drafts on non-OK/failed GET or PUT instead of silently clearing them; skip state updates after unmount. - Provider-level block/allow, autoLearn, and other model entries are preserved; an empty block+allow still removes only the selected model entry. - Ratchet the three now-clean dashboard-typecheck baseline entries. Compat-toggle and upstream-header paths are unchanged. * fix(dashboard): avoid lost update and surface failed param-filter saves (diegosouzapw#8910) The close-time save could clear the dirty flag for a payload snapshotted before the PUT resolved, silently discarding any keystroke that landed in that window. Track a monotonic draft revision and only acknowledge the revision that was actually written, re-running the save (bounded) otherwise. A failed save previously stayed dirty to 'retry on a later close', but reopening the popover reloaded server state and silently reverted the draft. Keep a dirty draft for the same provider/model on reopen and show a failure marker next to the saving indicator instead. * fix(dashboard): protect dirty param-filter drafts from load-effect clobber (diegosouzapw#8910) The retained-draft guard in the param-filters load effect required paramLoadedKeyRef to match the current target, but that ref was only assigned after a successful GET. Any draft typed before a successful load for that target was therefore unguarded, and the clean-slate write overwrote both the text and the dirty flag: - a draft typed while the INITIAL load GET was still in flight was overwritten and its dirty flag cleared, so the close-path save became a no-op and the keystrokes vanished with no feedback; - after a FAILED initial load, the retained draft was destroyed by the next successful reopen load — the exact moment the user reopens to retry — and the failure indicator was cleared as if the save had succeeded. Track the target on the dirty flag itself (paramDirtyKeyRef, set when the draft is marked dirty) instead of deriving it from a completed load, and re-check the guard after the GET await so a load result never overwrites text, clears dirty, or clears the failure indicator for a draft that is not on the server. * fix(dashboard): bind the param-filter save to the draft's own target (diegosouzapw#8910) saveModelParamFilters guarded on paramDirtyRef alone and read the providerId/modelId it closed over, never the target the draft was typed for. ModelCompatPopover is not always keyed by a stable identity (CompatibleModelsSection keys by `${alias}:${modelId}`, PassthroughModelsSection by the full model string, and providerId is threaded from route/page state), so a re-render can re-point a live, mounted popover at a different provider/model. If the old target's save had failed or never ran, the still-dirty draft was then PUT into the NEW target — writing a filter list under a model/provider the user never edited and destroying that target's real config. Replace the dirty flag / revision counter / dirty-key trio with a single ParamFilterDraft ref that carries the provider, model and both field values captured at edit time. The save drives its GET, PUT and payload from that draft instead of the current props, re-reads the ref after each await (restarting the attempt if the draft was replaced by one for another target), and only clears it when the exact draft object it wrote is still pending. Object identity replaces the revision counter, keeping the existing lost-update protection. A load no longer clears the draft or the failure indicator: a draft pending here belongs to another target and is still owed a write to it. An orphaned draft is therefore neither dropped nor redirected — it keeps its own provider/model, keeps the failure marker visible, and is retried by the next blur/close/unmount save. The cleanup effect also depends on the target key so re-pointing the popover flushes the old draft. * fix(dashboard): keep param-filter fields and drafts bound to their own target (diegosouzapw#8910) Two remaining defects of the diegosouzapw#8910 silent-data-loss family, both reached through the re-point path of a live ModelCompatPopover. 1. The inputs render blockText/allowText, whose only writer was the load effect — and that effect early-returned whenever a draft was dirty for the target. So re-pointing A -> B -> A left B's server values on screen under A, and the next keystroke snapshotted them into A's draft, persisting B's content into A's entry. The fields are now a function of the target: on return to a target with a pending draft the draft is restored into the inputs, and on a target with no draft the previous target's values are cleared instead of being left behind. An edit also no longer trusts the counterpart field unless the values on screen belong to the target being edited. 2. The pending draft lived in a single slot that every edit overwrote, so typing into a newly pointed target destroyed the previous target's unsaved work while the new target's successful save cleared the failure indicator — a green UI over data that was never written. Drafts are now keyed by provider/model; the save drains every pending draft against its own target, and the indicator reflects unsaved work across all targets rather than the last write. Regression tests: modelCompatPopover-param-filter-target-repoint.test.tsx (3 cases, RED at e6a005311, GREEN here). Scope limited to this component. * fix(dashboard): drain midflight param-filter drafts (diegosouzapw#8910) * fix(dashboard): serialize cross-row param-filter saves (diegosouzapw#8910) * docs(changelog): add fragment for diegosouzapw#9013 * chore: remove debug console.log and O5 test prefix --------- Co-authored-by: 千乘妍 (Xiaoyaner) <xiaoyaner0201@users.noreply.github.com> Co-authored-by: diegosouzapw <diegosouzapw@users.noreply.github.com>
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.
Summary
Fixes #8910 — model-level Allowed / Blocked params edits made in
ModelCompatPopovercould silently vanish, and a failed save could be presented as a success.The compatibility toggles use a separate immediate patch path and always persisted; the param-filter text fields kept edits in local component state and only wrote them from the input's
onBlur. Clicking outside closes and unmounts the portalled popover from the document-levelmousedownhandler, so theonBlurcommit never ran — the edit disappeared with no feedback. The save helper also cleared the dirty state without checking whether thePUThad actually succeeded.Working through the fix surfaced a family of related silent-data-loss defects on the same path, each covered by its own regression test.
What changed
src/app/(dashboard)/dashboard/providers/[id]/components/ModelCompatPopover.tsx:onBlur.PUTactually succeeded; a failedGETorPUTkeeps the edit and keeps the failure visible.PUTresolved, so a keystroke landing mid-flight is not discarded.GETwas still in flight, and a retained draft after a failed initial load.ModelCompatPopoveris not always keyed by a stable identity (CompatibleModelsSectionkeys by${alias}:${modelId},PassthroughModelsSectionby the full model string, andproviderIdis threaded from route/page state), so a re-render can re-point a live, mounted popover at a different provider/model. The save is now driven by aParamFilterDraftthat carries the provider, model and field values captured at edit time — never the props it happens to close over. This prevents writing one model's filter list into another model's config.Also: one entry removed from
config/quality/dashboard-typecheck-baseline.json(the touched file no longer produces those baseline errors), and a small binding change inCustomModelsSection.tsx.Test evidence
8 new regression test files under
src/app/(dashboard)/dashboard/providers/[id]/components/__tests__/(14 cases), each written RED against the preceding commit and GREEN here:modelCompatPopover-param-filters.test.tsxmodelCompatPopover-param-filter-load-clobber.test.tsxmodelCompatPopover-param-filter-cross-target.test.tsxmodelCompatPopover-param-filter-target-repoint.test.tsxmodelCompatPopover-param-filter-midflight-unmount.test.tsxmodelCompatPopover-param-filter-midflight-new-target.test.tsxmodelCompatPopover-param-filter-cross-instance.test.tsxmodelCompatPopover-param-filter-concurrency.test.tsxGates run on this exact tree (
537bbe1bda8278d3cf10dacfcaed16bec0aa29e3):All 9 failing files are pre-existing and unrelated to this change, and fail identically on the base branch:
dashboard/cache/__tests__(5 files —Cannot find module '@testing-library/dom'collection errors),discovery/DiscoveryPageClient(same collection error),webhooks/webhook-wizard("Next on step 2 posts kind:slack"),providers/[id]/ProviderDetailPageClient("kicks off provider data loading on mount",t.rich is not a function), andendpoint/ApiEndpointsTab(2 tests). All 9 param-filter /phase1dfiles pass.Scope / non-goals
No change to param-filter request semantics, no DB/schema/migration, no provider or network call change, no compatibility-policy redesign, no broad popover refactor. Existing compatibility toggles and upstream-header behavior are unchanged.
Opened as a Draft pending maintainer review. Branch is based on
release/v3.8.50; the accepted tree is537bbe1bda8278d3cf10dacfcaed16bec0aa29e3at commitdcddbe11cdf3964ee6289c0e1057aab8b41d33cd. A changelog fragment will be added as a follow-up commit once the PR number exists (perchangelog.d/README.md).