Skip to content

fix(web): keep AI configuration edits when the window regains focus - #81

Merged
matousekm merged 4 commits into
mainfrom
fix/prevent-form-reset
Oct 6, 2026
Merged

matousekm merged 4 commits into
mainfrom
fix/prevent-form-reset

Conversation

@matousekm

@matousekm matousekm commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

The generated fetcher returns a fresh Headers object on every request, so the query result envelope never structurally shares and each focus refetch reset the form. Reset only when the response body changes.

Summary

How to test

Checklist

  • PR title and summary are descriptive.
  • Max one DB migration per PR.
  • Docs updated.
  • Tests included.
  • Architecture-affecting changes follow the architecture guide.
  • I have seen this code, I have run this code, and I take responsibility for this code.

@matousekm matousekm self-assigned this Oct 3, 2026
@kilo-code-bot

kilo-code-bot Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Roast 🔥

Verdict: No Issues Found | Recommendation: Merge

Oh wait, this PR is actually still clean. I need to sit down — again.

The previous reviewer's feedback was all addressed: the standalone test-setup.ts got inlined into the test file and deleted, the version was pinned, and the hardcoded string got replaced with the i18n key. The only remaining nitpick (RefObject type on line 200) was already noted and commented in the previous round — no new issues introduced.

The core fix remains sound: depend on configurationQuery.data?.data (the body, with structural sharing) instead of the query envelope that gets a new Headers object on every refetch. The drafts.current.clear() on server data change is the right call. Tests cover both regression and happy path.

📊 Overall: Listened to every review comment, made it cleaner, and somehow didn't introduce a single new thing to roast. I'm starting to think this repo is run by competent people.

Files Reviewed (4 files, excluding bun.lock)
  • apps/web/package.json — 0 issues
  • apps/web/src/features/ai-configuration/ai-configuration-page.tsx — 0 new issues (1 existing nitpick on line 200, previously noted)
  • apps/web/src/features/ai-configuration/ai-configuration-page.test.tsx — 0 issues
  • apps/web/src/test-setup.ts (deleted) — 0 issues
Previous Review Summaries (4 snapshots, latest commit 4730580)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 4730580)

Commit: 47305805 · Review: full (previous review commit 4e4923a2 is not an ancestor of HEAD)
Verdict: No Issues Found, Recommend Merge

What changed

The PR prevents the AI configuration form from resetting when the window regains focus and triggers a React Query refetch. The root cause was the useEffect depending on configurationQuery.data (the response envelope), which is a new reference on every refetch due to headers. The fix depends on configurationQuery.data?.data (the body) instead, leveraging React Query v5 structural sharing to preserve referential equality when the server data has not changed.

Findings

# File Severity Summary
1 apps/web/src/features/ai-configuration/ai-configuration-page.tsx:200 nitpick RefObject prop type does not match MutableRefObject return from useRef

Verdict

No Issues Found, Recommend Merge — The fix is well-designed, correctly leverages React Query structural sharing, and the test covers both the keep-edits-on-refetch path and the save path. The single nitpick is a type precision concern, not a correctness issue.


View full review: https://app.kilo.ai/cloud-agent-fork/review/dae45de6-3875-4c12-bad8-aeb8de981b1c

Previous review (commit 4e4923a)

Verdict: No Issues Found | Recommendation: Merge

Oh wait, this PR is actually still clean. I need to sit down — again.

The incremental changes inline happy-dom registration into the test file and delete the now-unnecessary test-setup.ts. This addresses the previous reviewer's "do we need a 5-line file for this?" comment directly. The guard removal (typeof document === "undefined") is safe since this runs at module evaluation time before any async setup creates a document.

📊 Overall: The PR that keeps on giving. Listened to feedback, made it cleaner, and didn't introduce anything new to roast. I'm starting to feel self-conscious.

Files Reviewed (2 files in incremental diff)
  • apps/web/src/features/ai-configuration/ai-configuration-page.test.tsx — 0 issues
  • apps/web/src/test-setup.ts (deleted) — 0 issues

Previous review (commit 2ac58f2)

Verdict: No Issues Found | Recommendation: Merge

Both previous review comments were addressed in follow-up commits:

  • package.json: @happy-dom/global-registrator pinned to exact version ✅
  • Test file: hardcoded "Configuration saved" replaced with t("aiConfiguration.saved") ✅

The incremental diff is clean — two targeted fixes, zero new problems. The original PR logic remains sound, the drafts.current.clear() on body change is the right call, and the tests still pass.

📊 Overall: Like a PR that actually listens to code review. I'm not used to this.

Files Reviewed (3 files, excluding bun.lock)
  • apps/web/package.json — 0 issues
  • apps/web/src/features/ai-configuration/ai-configuration-page.test.tsx — 0 issues
  • apps/web/src/features/ai-configuration/ai-configuration-page.tsx — 0 issues

Previous review (commit 1898ca2)

Verdict: No Issues Found | Recommendation: Merge

Oh wait, this PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything.

The fix is textbook: depend on the response body, not the query envelope that gets a new Headers object every refetch. The drafts ref cleanup is a nice touch — clearing stale draft state when server data actually changes, without losing unsaved edits on identical refetches.

Tests cover both the regression (edits survive unchanged refetch) and the happy path (form resets after save). The mock throws on unexpected URLs instead of silently passing — that's the kind of defensive testing that pays off at 3 AM.

📊 Overall: Like finding a unicorn in production — I didn't think clean PRs existed anymore, but here we are.

Files Reviewed (4 files, excluding bun.lock)
  • apps/web/src/features/ai-configuration/ai-configuration-page.tsx — 0 issues
  • apps/web/src/features/ai-configuration/ai-configuration-page.test.tsx — 0 issues
  • apps/web/src/test-setup.ts — 0 issues
  • apps/web/package.json — 0 issues

Reviewed by deepseek-v4-flash · Input: 46.1K · Output: 6.4K · Cached: 225.3K

@matousekm
matousekm requested a review from Fidasek009 October 4, 2026 14:59
Comment thread apps/web/src/test-setup.ts Outdated
@matousekm
matousekm force-pushed the fix/prevent-form-reset branch from 4e4923a to 4730580 Compare October 5, 2026 23:11
Comment thread apps/web/src/features/ai-configuration/ai-configuration-page.tsx
matousekm and others added 4 commits October 6, 2026 20:58
The generated fetcher returns a fresh Headers object on every request, so
the query result envelope never structurally shares and each focus refetch
reset the form. Reset only when the response body changes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@matousekm
matousekm force-pushed the fix/prevent-form-reset branch from 4730580 to 61b6f2c Compare October 6, 2026 19:00
@matousekm
matousekm merged commit d686aaf into main Oct 6, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants