feat(web): remember New Loop choices#56
Conversation
Mingholy
left a comment
There was a problem hiding this comment.
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 afterawait onCreate(...)succeeds, so failed creations don't pollute memory. The internal try/catch makes localStorage truly best-effort.- The
NewLoopStoragetype (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:
-
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. -
Nit:
RepoSelectactiveIdx scrollIntoView effect lists[activeIdx, open]as deps but theopenguard 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 addedcontextto theonCreatetype signaturedf7e5ca 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
onCreatesignature 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.
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>
fc10dbf to
4cd9a44
Compare
|
已 rebase 到最新 main( Rebase 结果无冲突。Git 能够自动 apply PR 的单个提交(
Rebase 后的
额外修复在 rebase 过程中发现 PR 的
TypeScript 类型检查 ( |
最终 Review 结论 ·
|
Summary
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.
Test plan
cd web && bunx tsc -bbun run buildgit diff --checknewLoopMemorytests were run during implementation.web, andserverpackage manifests do not define alintscript.