Skip to content

refactor(gui): stabilize model feedback callback dependencies - #3984

Draft
yansigit wants to merge 1 commit into
lidge-jun:devfrom
yansigit:codex/upstream-model-feedback-20260908
Draft

refactor(gui): stabilize model feedback callback dependencies#3984
yansigit wants to merge 1 commit into
lidge-jun:devfrom
yansigit:codex/upstream-model-feedback-20260908

Conversation

@yansigit

@yansigit yansigit commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Wrap the setter-only feedback publisher in useCallback and include it in the display-name save callback dependencies. This makes the callback contract explicit without changing displayed feedback or save behavior.

No user-facing behavior change; existing documentation remains accurate. Screenshot shows the verified existing editor.

Verification

Root bun run typecheck, GUI bun run lint, and bun run build passed. GUI tests: 1921 passed, zero failures. Browser verification saved and reopened a friendly name using synthetic API data.

All runtime checks used a fresh temporary OPENCODEX_HOME and alternate port; production config fingerprint and backup inventory remained unchanged. Full root-suite and review-readiness gates have not been completed for this head; this is intentionally a draft.

Synthetic review screenshot

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Added/updated regression coverage or verified existing coverage for the affected behavior.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Review readiness

  • Local CI green.
  • Branch on the latest dev commit.
  • All correct Codex and CodeRabbit findings fixed.
  • Ready-for-review confirmation.

Co-authored-by: SB Yoon 44089734+yansigit@users.noreply.github.com
Co-authored-by: Yumi automation@sbyoon.com

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • All CI tests are green on my local testing.

  • I pushed my PR to the latest dev commit.

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

Extracted and adapted from fork commit 4d36c91.

Co-authored-by: Yumi <automation@sbyoon.com>
@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). intake: hygiene-blocked Deterministic PR hygiene checks failed labels Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

⚠️ Deterministic hygiene checks failed.

  • missing_regression_test — Behavior changed under src/ or gui/src/ without a test change. Add focused coverage or obtain test-exception-approved.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • hygiene: missing_regression_test.

What to do

  • Fix missing_regression_test — Behavior changed under src/ or gui/src/ without a test change. Add focused coverage or obtain test-exception-approved.
  • Tick all four boxes in the PR description once you're done (currently 0/4).

Review readiness checklist

  • ⬜ All CI tests are green on my local testing.
  • ⬜ I pushed my PR to the latest dev commit.
  • ⬜ I resolved all correct Codex and CodeRabbit findings.
  • ⬜ My PR is ready for review.

0/4 boxes ticked.

This pull request was already a draft. Its draft status will be preserved after every issue above is resolved.
@yansigit Tick the boxes once your local CI is green, your branch is on the latest dev commit, and every correct Codex and CodeRabbit finding is resolved.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 36 / 80

이 PR은 gui/src/pages/Models.tsx에서 publishFeedbackuseCallback으로 감싸고, saveDisplayName effect 의존성 배열에 넣습니다. 의도 설명은 분명합니다. toast generation(feedbackGen)을 올리는 함수가 매 렌더마다 새 참조면, 그걸 dep에 넣은 effect가 불필요하게 다시 돌 수 있다는 이야기입니다. 지금 dev HEAD c15662855의 같은 파일 721–731행 근처에는 이미 반대 방향의 주석이 있습니다. plain async loader를 useCallback으로 감싸 exhaustive-deps를 채우면 PreserveManualMemo·Immutability·EffectSetState 경고가 한 개에서 다섯 개로 늘어난다고 2026-08-27에 검증해 두었고, gui/.oxlintrc.json / gui/doctor.config.json으로 이 파일 규칙을 눌러 둔 상태입니다. 그래서 “dep를 채우려고 useCallback을 추가한다”는 이 PR의 방향이, 파일에 이미 적은 react-compiler hygiene과 충돌할 여지가 큽니다. 게이트도 이미 intake: hygiene-blocked이고, hygiene 봇이 missing_regression_test(gui/src/ 동작 변경인데 테스트 없음)로 막았습니다. 스크린샷 바이너리만 추가됐고 회귀 테스트는 없습니다. types/config 분할과 무관합니다.

라인 gui/src/pages/Models.tsx · publishFeedback useCallback - 빈 deps []로 감쌉니다. setState만 쓰니 참조 안정화 자체는 맞지만, 이 파일의 PreserveManualMemo 예외 전략과 같은 패턴인지 먼저 확인해야 합니다.
라인 gui/src/pages/Models.tsx · saveDisplayName deps - publishFeedback을 배열에 추가합니다. 함수를 안정화하지 않은 채 넣으면 effect churn이 생기고, 안정화하면 compiler 경고 표면이 늘 수 있습니다.
경로 intake: hygiene-blocked / missing_regression_test - GUI 동작 변경인데 테스트가 없습니다. test-exception-approved 또는 Models toast/feedback 포커스 테스트가 필요합니다.
경로 assets/pr-screenshots/model-feedback-review.png - 시각 증거는 있으나 hygiene의 regression-test 요구를 대체하지 않습니다.
경로 enforce-target - 현재 fail입니다. draft 체크리스트도 0/4입니다.

메인테이너의 판단이 필요한 지점

  • 이 파일을 계속 “수동 memo 예외”로 둘지, feedback만 예외적으로 useCallback을 허용할지
  • missing_regression_test를 테스트 추가로 풀지, test-exception-approved로 예외할지
  • compiler 경고가 다시 늘면 이 PR을 닫고 의존성 주석만 보강하는 편이 나을지

너의 추천
지금 상태로는 머지하지 마세요. hygiene(missing_regression_test)를 먼저 풀고, bun run lint / react-compiler 경고가 HEAD 주석과 모순되지 않는지 로그를 PR에 붙이세요. 경고가 늘면 close-don't-rebase가 맞습니다. toast 조기 dismiss 버그가 재현되면 그 재현 테스트를 핵심으로 다시 열면 됩니다.

이 댓글은 grok-bot이 작성했습니다

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). intake: hygiene-blocked Deterministic PR hygiene checks failed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants