fix: preserve app-server read across Models tab switches; bound persisted requestedModel

[luvs01]

Summary

• Models picker cleanup no longer cancels the in-flight app-server status read when switching tabs. cancelAppServerRead was called from the picker effect cleanup, which runs whenever apiBase or catalogActive changes; the status read is owned by its own effect and must survive tab switches.

• normalizeUsageEntry now persists requestedModel in a bounded, collision-resistant form: selectors up to 130 chars persist verbatim, and longer selectors persist as a bounded prefix plus a short SHA-256 digest of the full selector. Model selectors are not length-bound at admission (configured and discovered model ids can far exceed 130 chars), so a plain truncation could merge two distinct selectors that share the prefix; the digest keeps them distinguishable. The one exact-match consumer — the request-history requested_model = ? filter — encodes lookup input through the same exported function, and the encoding is idempotent so rows re-normalized on read are unchanged.

• resolvedModel is intentionally left unbounded: it is a server-resolved catalog value, not client-supplied input like requestedModel, so it does not share the same abuse path.

Verification

• bun test tests/usage/usage-log.test.ts tests/usage/request-history-index.test.ts — 65 pass / 0 fail (includes the new shared-prefix regression tests).
• cd gui && bun test tests/models-status-toast.test.tsx — 22 pass / 0 fail (includes the new tab-switch regression test).
• bun x tsc --noEmit clean; cd gui && bun x tsc -b clean.
• GUI change is behavior-only (no visual diff), so no screenshot is attached.

Checklist

• Scope stays focused and avoids unrelated cleanup.
• Docs or release notes were updated when needed.
• Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Summary by CodeRabbit

• Bug Fixes

  • Switching between Models tabs no longer interrupts an in-progress status check.
  • Usage logs now limit persisted requested model names to 130 characters, helping prevent oversized log entries.

• Tests

  • Added coverage for uninterrupted status checks during tab switching.
  • Added coverage confirming model-name length limits and compact usage-log output.

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.

UI evidence

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 0826aaf7-6cbc-4d0a-af3d-cfb05f4d625d

📥 Commits

Reviewing files that changed from the base of the PR and between 64bad3e and f7fbe3b.

📒 Files selected for processing (4)

• gui/src/pages/Models.tsx
• gui/tests/models-status-toast.test.tsx
• src/usage/log.ts
• tests/usage/usage-log.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.

---

📝 Walkthrough

Walkthrough

Changes

Models read lifecycle

Layer / File(s)	Summary
Preserve app-server status reads gui/src/pages/Models.tsx, gui/tests/models-status-toast.test.tsx	The Models effect cleanup no longer cancels the app-server read. The test verifies that switching to the Combos tab leaves the pending read active and renders the stale banner after completion.

Usage log bounds

Layer / File(s)	Summary
Bound persisted model selectors src/usage/log.ts, tests/usage/usage-log.test.ts	Usage normalization truncates non-empty requestedModel values to 130 characters. The test verifies the persisted selector and keeps the log below 1024 bytes.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: lidge-jun

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies both primary changes: preserving the app-server read across Models tab switches and bounding persisted requestedModel values. It is concise, specific, and directly related…

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

🧪 Generate unit tests (beta)

• Create a new PR

🛠️ Fix failing CI checks 💡

• Commit to this branch
• Create a new PR

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[Ingwannu]

The Models-tab cancellation fix looks sound, but the 130-character persistence bound is not safe as written.

The PR description assumes provider and model segments are each limited to 64 characters. The current config/model admission path does not enforce those limits: provider names are syntax-validated without a 64-character maximum, and configured or upstream model IDs can be longer. Truncating requestedModel in normalizeUsageEntry can therefore turn a valid selector into a different identity, breaking the exact-match rollups that the comment says this field must preserve (and potentially colliding two long selectors).

Please either bind this persistence limit to a shared model-selector admission contract that is enforced before routing, or persist a bounded collision-resistant representation and update every exact-match consumer to use the same representation. Add a regression with two valid selectors that share the first 130 characters and prove they remain distinguishable. The two hosted workflows are authorized, but this exact head should not be approved yet.

[github-actions]

✅ Deterministic PR hygiene checks passed.

[github-actions]

✅ READY

• all PR quality gates passed; the review readiness checklist is complete.

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.

✅ 4/4 boxes ticked.

Automatic ready-for-review conversion failed; please mark the pull request ready manually if it is still a draft.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers notified: @lidge-jun @Ingwannu

Hygiene

✅ Deterministic PR hygiene checks passed.

[lidge-jun]

리뷰 · 우선순위 56 / 80

이 PR은 서로 다른 두 군데의 “너무 일찍 끊거나, 너무 길게 남기는” 버그를 고칩니다. 첫째, Models 화면에서 탭을 바꿀 때 picker 정리 코드가 cancelAppServerRead까지 같이 호출해서, 아직 끝나지 않은 app-server 상태 조회가 취소되던 문제를 제거했습니다. 상태 조회는 자기 effect가 수명을 갖고 있고, 탭 전환(catalogActive 변경)과 묶이면 안 됩니다. 둘째, usage 로그에 남는 requestedModel이 한없이 길어질 수 있어 130자로 잘라 저장합니다. 정상 두 조각 셀렉터(64/64)는 129자라서 그 길이는 살리고, 비정상으로 큰 값만 막겠다는 의도입니다. GUI 탭 전환 회귀 테스트와 usage 바운드 회귀 테스트가 각각 붙어 있고, base는 dev입니다.

라인 - gui/src/pages/Models.tsx picker cleanup: cancelAppServerRead를 뺀 방향은 맞습니다. 상태 조회는 apiBase/restartEpoch effect의 cleanup이 담당하고, 탭 전환은 picker flight만 끊는 편이 맞습니다. 여기 추가 수정은 필요 없어 보입니다.
라인 - src/usage/log.ts normalizeUsageEntry: requestedModel만 130자로 자르고 resolvedModel은 그대로입니다. 같은 경로로 비정상으로 큰 문자열이 들어오면 디스크/행 크기 문제는 한쪽만 막힌 상태입니다. 의도적으로 lookup 키라 안 자른 건지, 빠진 건지 한 줄 확인이 필요합니다.
라인 - PR 게이트: enforce-target이 “UI screenshot required”로 실패했고 PR은 draft이며 readiness checklist가 0/4입니다. 본문에는 “동작만 바뀌고 화면 차이는 없다”고 적혀 있습니다. 스크린샷 면제(waiver)가 가능한지, 아니면 배너가 뜨는 한 컷이라도 넣을지 정리해야 merge가 열립니다.
라인 - 한 PR에 GUI 취소 타이밍 수정과 usage persist 바운드가 같이 들어 있습니다. 둘 다 작고 테스트가 있어 묶어도 되지만, 되돌리기/bisect를 생각하면 분리도 가능합니다. 필수는 아닙니다.

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

동작-only GUI 수정에 스크린샷 게이트를 어떻게 적용할지(면제 vs 최소 캡처), 그리고 resolvedModel을 계속 무제한으로 둘지입니다. 코드 방향 자체는 보수적이고 회귀 테스트도 핵심 경로를 덮습니다.

너의 추천

앱서버 읽기 취소 분리와 requestedModel 130자 바운드는 머지해도 좋아 보입니다. draft/스크린샷 게이트와 checklist만 정리한 뒤 ready로 올리면 됩니다. resolvedModel은 “왜 안 자르는지”를 PR 본문이나 코드 주석에 한 줄만 남기거나, 같은 위협이면 같은 상한을 맞추세요.

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

[luvs01]

@Ingwannu Reworked the persistence bound per your review.

What changed

• normalizeUsageEntry no longer truncates requestedModel. Selectors up to 130 chars persist verbatim; longer ones persist as a bounded prefix plus a 16-hex SHA-256 digest of the full selector (encodePersistedRequestedModel in src/usage/log.ts), so two distinct selectors that share the prefix can never collapse into one identity.
• The one exact-match consumer — the request-history requested_model = ? filter in src/routing/history/indexer.ts — encodes lookup input through the same exported function, so filtering by either the raw selector or the surfaced persisted form matches correctly.
• The encoding is idempotent (encoded forms fit the bound), which matters because normalizeUsageEntry also runs on every read of legacy rows.

Why this option over an admission contract
Binding the 130 bound to selector admission would have required enforcing a max length on the wire model field across every inbound endpoint (chat completions, claude-messages, responses, combo, compact paths) and tightening config-load validation for currently unbounded fields (defaultModel, models[], modelAliases, provider alias). Worse, the model segment legitimately reaches MODEL_DISCOVERY_MAX_MODEL_ID_LENGTH (1024), so a 64-char segment limit would reject selectors that route successfully today — a much larger blast radius than the persistence-layer fix. The digest form keeps every valid selector distinguishable with no routing-behavior change.

Regression coverage

• usage-log.test.ts: two valid selectors sharing the first 130 chars persist to distinct values; short selectors persist verbatim; re-normalization is a no-op.
• request-history-index.test.ts: the requestedModel filter resolves each of two shared-prefix selectors to its own row, and the surfaced encoded form round-trips through the filter.

Local: bun test on touched files 65+22 pass / 0 fail; bun x tsc --noEmit and cd gui && bun x tsc -b clean. The two hosted workflows are awaiting approval on this head.

[luvs01]

Consolidated into #5556 as a single related-function aggregate.

Source head: 0b02a36ca57654683e857c91d0231730ef185041. Replacement head: d3589638a877530f89c111d3dba69d7a76908939.

Both unique GUI-lifecycle and persistent-selector commits match carried 5563577 and c8a9d1a by stable patch IDs; the two dev merges have no additional remerge-resolution delta. The full-selector digest and matching history filter are retained, as is the independently owned Models status read. Models tests passed 22/22; actual-component headless DOM and screenshot evidence is linked in #5556. The 1,500-row/15-page pagination fixture now writes identical normalized rows once, preserving every assertion and its five-second bound; the previously failing case and related append/filter cases passed 4/4. Full final suite, hosted CI and required review remain pending.

Closing this duplicate standalone review entry as part of the requested consolidation after verifying coverage. This is not a merge or release claim; remaining integration checks and reviews are tracked on the replacement. Original branches are retained.