Skip to content

feat(web): remember New Loop choices#56

Open
Mingholy wants to merge 2 commits into
mainfrom
feat/ft-10-newloop-remember
Open

feat(web): remember New Loop choices#56
Mingholy wants to merge 2 commits into
mainfrom
feat/ft-10-newloop-remember

Conversation

@Mingholy

@Mingholy Mingholy commented Jun 9, 2026

Copy link
Copy Markdown
Owner

Summary

  • Remember the last successful New Loop repo/profile/vault choices in local storage.
  • Restore remembered choices only when they still exist in the freshly loaded options.
  • Restore the reusable repo combobox selection flow and cover the memory logic with focused tests.

Screenshot

Reopening the New Loop dialog restores the previously used selections instead of resetting to defaults: profiles mode-oncall + sre (see "Selected: mode-oncall, sre") and Context Cached (the default is Latest). Remembered values are only restored when they still exist in the freshly loaded options.

after

Test plan

  • cd web && bunx tsc -b
  • bun run build
  • git diff --check
  • Focused newLoopMemory tests were run during implementation.
  • Lint not run: root, web, and server package manifests do not define a lint script.

@Mingholy Mingholy left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: PR #56

Summary

This PR adds localStorage-backed memory for the New Loop dialog so that repo, profile, and vault selections persist across dialog opens. It also upgrades the repo selector from a plain <select> to a searchable combobox (RepoSelect) with fuzzy matching. The memory logic is cleanly extracted into newLoopMemory.ts with focused unit tests.

Implementation Quality

Architecture is clean. The three-layer design (read snapshot -> resolve against live data -> write on success) is the right pattern for this. Key design decisions are correct:

  • readNewLoopMemory() produces a snapshot object once, shared across all three async .then() callbacks. This avoids inconsistency if localStorage changed between callbacks.
  • Stored values are validated against the freshly-fetched server state before restoring (repo must still exist, profiles filtered against available set, vault checked against current list). This prevents stale references.
  • writeNewLoopMemory() is called only after await onCreate(...) succeeds, so failed creations don't pollute memory. The internal try/catch makes localStorage truly best-effort.
  • The NewLoopStorage type (Pick<Storage, "getItem" | "setItem">) is a nice minimal interface that makes tests clean without mocking globals.

resolveStoredProfiles has a subtle but correct edge case. When stored profiles exist but are all stale (none in available), the condition restored.length > 0 || memory.profiles.length === 0 is false, so it falls through to defaults. This is the right behavior: if the user's previous choices all became invalid, fall back to defaults rather than an empty set.

resolveStoredVault returning "default" for empty vault lists matches the original behavior (the <select> shows a hardcoded default option when vaults.length === 0).

RepoSelect is well-built: keyboard navigation (arrow keys + Enter + Escape), fuzzy search with consecutive/boundary bonuses, proper ARIA attributes (role="listbox", role="option", aria-selected, aria-expanded), scroll-into-view on active index change, and the onMouseDown preventDefault to keep input focused — all standard combobox patterns done right.

Code Quality

Code is clean, well-structured, and consistent with the existing codebase style. Test coverage is good for the memory module with meaningful edge cases (stale profiles, invalid JSON, empty storage).

Two minor notes:

  1. No tests for fuzzy.ts. The function is simple enough to verify by inspection, but a handful of edge cases (empty strings, case insensitivity, boundary bonuses, no-match returns null) would be cheap insurance.

  2. Nit: RepoSelect activeIdx scrollIntoView effect lists [activeIdx, open] as deps but the open guard means it's a no-op when closed. Not a bug, just slightly unnecessary re-runs.

Merge Conflict Warning

This branch will conflict with current main. The merge-base is f145090 but main has two commits that modified NewLoopDialog.tsx since then:

  • 2459858 feat(loop): context freshness via git refs — added the context/freshness UI (radio group + custom ref input) and added context to the onCreate type signature
  • df7e5ca feat(loop): auto-name untitled loops after first turn — moved the Name field to the bottom of the form and changed its hint text

Specifically:

  • The PR's onCreate signature is { title, repo, profiles, vault } but main now has { title, repo, profiles, vault, context }.
  • The PR has the Name field first; main has it last.
  • The Context/freshness section in main is not present in the PR branch at all.

A rebase onto current main is needed before merge. The conflicts should be straightforward to resolve — the memory and RepoSelect features are additive and don't interact with the context/freshness feature.

Security

No concerns. The PR only stores non-sensitive UI preferences (repo name, profile names, vault name) in localStorage. Values are used as React props, never dangerously injected as HTML. JSON parsing in readProfileList properly validates the shape before returning.

Verdict

Needs Changes — solely for the rebase onto current main to pick up the context/freshness and auto-name changes. The implementation itself is solid: well-separated memory logic, correct stale-value handling, good test coverage, and a proper searchable combobox. After rebase, this is an LGTM.

Mingholy and others added 2 commits June 9, 2026 13:57
Extend newLoopMemory to also remember the user's last-chosen freshness
(latest/cached/custom) so it's restored on next open, consistent with
how repo/profiles/vault are already persisted.

- Add `freshness` key to NEW_LOOP_MEMORY_KEYS
- Add `readFreshness` + `resolveStoredFreshness` helpers with validation
- Update `writeNewLoopMemory` signature to include freshness
- Wire up in NewLoopDialog: restore freshness on mount, save on create
- Update tests: fix writeNewLoopMemory call-site, add freshness round-trip cases

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Mingholy Mingholy force-pushed the feat/ft-10-newloop-remember branch from fc10dbf to 4cd9a44 Compare June 9, 2026 06:03
@Mingholy

Mingholy commented Jun 9, 2026

Copy link
Copy Markdown
Owner Author

已 rebase 到最新 main(846982a — chore: bump to v0.1.62),并 force push 更新 PR。

Rebase 结果

无冲突。Git 能够自动 apply PR 的单个提交(feat(web): remember new loop choices)到最新 main 上,原因是:

  • PR 修改的是 NewLoopDialog.tsxuseEffect 初始化块、Repo field(替换为 RepoSelect combobox)和 submit handler
  • main 上的两个冲突提交修改的是不同的代码区域:context freshness 新增在 Vault field 之后,Name field 移到表单底部

Rebase 后的 NewLoopDialog.tsx 同时包含了所有功能:

  • RepoSelect combobox(PR 新增)
  • localStorage memory 逻辑(PR 新增)
  • context/freshness radio group(main 新增)
  • Name 字段在表单底部(main 调整)
  • onCreate 签名包含 context?: string(main 新增)

额外修复

在 rebase 过程中发现 PR 的 writeNewLoopMemory 调用没有保存 freshness 选择,与 repo/profiles/vault 的记忆逻辑不一致。补充了一个额外提交(4cd9a44):

  • newLoopMemory.ts:新增 freshness key、readFreshness 私有函数、resolveStoredFreshness 导出函数;writeNewLoopMemory 签名扩展 freshness 字段
  • NewLoopDialog.tsx:mount 时从 storage 恢复 freshness 初始值;create 时写入 freshness
  • newLoopMemory.test.ts:修复 writeNewLoopMemory 调用(新增必填字段),添加 freshness 读写 round-trip 测试(6 tests, all pass)

TypeScript 类型检查 (bunx tsc --noEmit) 通过,无错误。

@Mingholy

Mingholy commented Jun 9, 2026

Copy link
Copy Markdown
Owner Author

最终 Review 结论 · ⚠️ 可合并(有 nit)

意图与达成度:高。三层设计干净 —— readNewLoopMemory() mount 时取快照 → 各 resolveStored* 针对实时服务端数据校验后还原 → writeNewLoopMemory() 仅在 await onCreate() 成功后写入。陈旧值(已删除的 repo/profile/vault)优雅回退默认值,不会破坏对话框。bun test web/test/newLoopMemory.test.ts 通过(6 pass)。

历史 review comment

  • rebase 到当前 main(拿到 context/freshness + auto-name 改动)✅ 已 rebase 到 846982a
  • freshness 未被记忆、与 repo/profiles/vault 不一致 ✅ 已在 4cd9a44 修复并补 round-trip 测试。
  • fuzzy.ts 补单测 / RepoSelect scrollIntoView nit —— 原 review 标注为 optional,未处理,可接受。

安全:clean。localStorage 仅存非敏感 UI 偏好(repo 名 / profile 名数组 / vault 名 / freshness 枚举),无凭证/token/路径落地;所有还原值使用前都对服务端实时数据做成员校验,不信任持久化数据直接注入请求;无 dangerouslySetInnerHTML

建议合并前顺手处理(Low)NewLoopDialog.tsx:28-29,111-116 —— 记忆 freshness="custom" 但不记忆具体 customRef,下次打开恢复成 custom 却空输入框,submit 会静默回退到 HEAD。建议要么一并记忆 customRef,要么 customRef 为空时不记成 custom。

:隔离 worktree 因 @types/react 未装无法本地全量 typecheck,请确认 CI typecheck 绿。

无 Critical/High/Medium 问题。

🤖 Generated with Claude Code

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.

1 participant