feat(domain): T-223 settings KV + persist current_project_id (closes #91)#117
Merged
Conversation
closes #91) Add a generic key-value `settings` table (V002 migration) with a new `domain/settings/` bounded context (`SettingsRepository` port + libsql adapter), expose `settings_get` / `settings_set` IPC commands, and wire the React boot flow to restore the selected project tab across restarts. Domain `service` validates keys (non-empty, ≤256 chars, no control / bidi) and values (no NUL, no bidi, ≤64 KB) so libsql sees nothing dangerous. Adapter folds every libsql failure into a stable `DomainError::IllegalState("database unavailable")` per ADR-016 and emits the raw text via `tracing::error!` for Sentry. Frontend restores the persisted id AFTER `setProjects` so the store's `reconcileSelectedId` cannot drop a stale id before the existence check falls back to the first project. A single `useProjectStore.subscribe` listener mounted at App level persists every `selectedId` change without touching `ProjectTabBar` or `useProjectCreateForm`. Tests: 17 service-layer + 9 libsql integration + 2 V002 pool + 2 IPC anchor + 14 helper vitest + 6 App-level T-223 cases. Cargo 274/274, vitest 330/330, clippy 0 errors, tsc clean.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughEnd-to-end persistence for selected project ID: adds V002 settings KV migration, settings domain/service with validation, Libsql adapter, DI wiring, Tauri IPC commands and typed contract, frontend IPC helpers, App boot restoration and persistence-on-change, and backend/frontend tests. ChangesSettings KV Persistence & Project Selection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/projects/persist-selected-project-id.test.ts`:
- Around line 1-24: The test creates a spy via vi.spyOn(console, "warn") but
only clears it, not restoring it, which can leak into other tests; add an
afterAll hook that calls consoleWarnSpy.mockRestore() (referencing the
consoleWarnSpy identifier in persist-selected-project-id.test.ts) to fully
restore console.warn after the suite finishes, leaving the existing
beforeEach/afterEach resets intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08ecf3c4-7f3c-447e-a8a9-547299a16fa6
📒 Files selected for processing (21)
CHANGELOG.mdsrc-tauri/src/application/di.rssrc-tauri/src/bin/specta-export.rssrc-tauri/src/domain/mod.rssrc-tauri/src/domain/settings/mod.rssrc-tauri/src/domain/settings/model.rssrc-tauri/src/domain/settings/ports.rssrc-tauri/src/domain/settings/service.rssrc-tauri/src/infrastructure/persistence/migrations/V002__settings.sqlsrc-tauri/src/infrastructure/persistence/mod.rssrc-tauri/src/infrastructure/persistence/pool.rssrc-tauri/src/infrastructure/persistence/settings_repo.rssrc-tauri/src/interfaces/tauri_commands/mod.rssrc-tauri/src/interfaces/tauri_commands/register.rssrc-tauri/src/interfaces/tauri_commands/settings.rssrc/App.test.tsxsrc/App.tsxsrc/features/projects/persist-selected-project-id.test.tssrc/features/projects/persist-selected-project-id.tssrc/shared/ipc/settings.tssrc/shared/types/commands.ts
Contributor
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 21 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/features/projects/persist-selected-project-id.ts">
<violation number="1" location="src/features/projects/persist-selected-project-id.ts:38">
P2: Use a safe-integer check when restoring the persisted project id; otherwise large numeric strings can be rounded and restore the wrong project.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d94445888f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ore console spy Address PR #117 bot review (CodeRabbit / cubic / Codex): - Serialise concurrent `persistSelectedProjectId` writes through a module-scoped promise chain so rapid `select(a)` then `select(b)` clicks cannot overwrite the persisted id with a stale value when the libsql layer races on per-call connections. - Switch `Number.isInteger` to `Number.isSafeInteger` so a stored id beyond 2^53 cannot silently round and restore the wrong project. - Add `afterAll` + `mockRestore` on the `console.warn` spy so it does not leak across vitest files in the same worker. - Expose `resetWriteChainForTesting` test seam so `beforeEach` cases isolate the chain across runs. Tests: 14/14 helper cases pass (incl. new chain-ordering and unsafe-int regression cases). Full vitest 333/333. tsc clean. oxlint 0 errors.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/features/projects/persist-selected-project-id.ts (1)
62-78: ⚡ Quick winSplit parsing from I/O to satisfy the statement-count gate.
Line 62 currently combines fetch, parsing, validation, and fallback in one function and trips
max-statements. Extracting value parsing into a helper keeps behavior identical and clears the lint warning.♻️ Proposed refactor
+function parsePersistedProjectId(value: string | null): number | undefined { + if (value === null) { + return undefined; + } + const parsed = Number(value); + if (!Number.isSafeInteger(parsed) || parsed < MIN_VALID_PROJECT_ID) { + return undefined; + } + return parsed; +} + export async function loadPersistedSelectedProjectId(): Promise<number | undefined> { try { const value = await invokeSettingsGet({ key: SELECTED_PROJECT_ID_KEY }); - if (value === null) { - return undefined; - } - const parsed = Number(value); - if (!Number.isSafeInteger(parsed) || parsed < MIN_VALID_PROJECT_ID) { - return undefined; - } - return parsed; + return parsePersistedProjectId(value); } catch (error: unknown) { const message = error instanceof Error ? error.message : String(error); console.warn(`[Forgent] Failed to load persisted selected project id: ${message}`); return undefined; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/projects/persist-selected-project-id.ts` around lines 62 - 78, Extract the parsing/validation logic out of loadPersistedSelectedProjectId into a small pure helper (e.g., parsePersistedSelectedProjectId) so the function only does I/O and the helper only does parsing; keep the same behavior: accept the value returned from invokeSettingsGet({ key: SELECTED_PROJECT_ID_KEY }), return undefined if value is null, convert with Number(value), return undefined unless Number.isSafeInteger(parsed) and parsed >= MIN_VALID_PROJECT_ID, otherwise return the parsed number; ensure loadPersistedSelectedProjectId wraps only the invokeSettingsGet call in try/catch and delegates parsing to parsePersistedSelectedProjectId, preserving the existing error logging using the same message text.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 15: The Unreleased section contains a duplicate heading "### Fixed";
remove the duplication by either merging the content under the existing "###
Fixed" heading or renaming the second heading to a qualifier (e.g., "### Fixed
(internal)") so there is only one canonical "### Fixed" heading in the
Unreleased block; update the heading text where the second "### Fixed" appears
to the chosen name and move any entries under it into the consolidated section
if you merge.
---
Nitpick comments:
In `@src/features/projects/persist-selected-project-id.ts`:
- Around line 62-78: Extract the parsing/validation logic out of
loadPersistedSelectedProjectId into a small pure helper (e.g.,
parsePersistedSelectedProjectId) so the function only does I/O and the helper
only does parsing; keep the same behavior: accept the value returned from
invokeSettingsGet({ key: SELECTED_PROJECT_ID_KEY }), return undefined if value
is null, convert with Number(value), return undefined unless
Number.isSafeInteger(parsed) and parsed >= MIN_VALID_PROJECT_ID, otherwise
return the parsed number; ensure loadPersistedSelectedProjectId wraps only the
invokeSettingsGet call in try/catch and delegates parsing to
parsePersistedSelectedProjectId, preserving the existing error logging using the
same message text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b120d12-7856-4795-9c8f-d57bdf63bc98
📒 Files selected for processing (3)
CHANGELOG.mdsrc/features/projects/persist-selected-project-id.test.tssrc/features/projects/persist-selected-project-id.ts
PR #117 CodeRabbit flagged MD024 (multiple headings with same content) on the second `### Fixed` under `[Unreleased]`. Merge the new T-223 fix entry under the existing `### Fixed` section and keep the T-223 add entry under the existing `### Added` section so each Keep-a-Changelog category appears exactly once per version.
CI frontend job runs `pnpm exec oxlint . --max-warnings=0` and was failing with 39 style warnings introduced by T-223. Local `oxlint .` (no flag) only prints them, so they slipped through pre-push. Resolutions (all behavioural-equivalent, no scope creep): - Convert multi-line `//` comment blocks to `/* ... */` so `eslint(capitalized-comments)` no longer fires on continuation lines. - Extract `parsePersistedId` from `loadPersistedSelectedProjectId` so the entrypoint stays under the `max-statements: 10` ceiling. - Extract `resolveBootSelection` from `App.tsx`'s boot `.then(...)` arrow for the same reason. - Move every magic literal in the test file into named `*_ID` / `*_CALL` / `*_DELAY_MS` constants and helper `expectInvokeNthCall` / `seedDeferredFirstWriteThenResolved` to eliminate `no-magic-numbers` + the residual `max-statements`. - Reorder imports so `Multiple`-syntax declarations precede `Single`-syntax ones (sort-imports). - Rename `T` generic to `TValue` (id-length minimum 2). - Tighten the persist `useEffect` arrow body to satisfy `arrow-body-style`. Tests: vitest 333/333, tsc clean, `pnpm exec oxlint . --max-warnings=0` clean (was 39 warnings).
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
settingsKV table (V002 migration) and a newdomain/settings/bounded context withSettingsRepositoryport + libsql adapter.settings_get/settings_setIPC commands (registered + specta-anchored + ts contract regenerated).ui.selected_project_idaftersetProjects(soreconcileSelectedIdcannot drop a stale id) and persist everyselectedIdchange via a singleuseProjectStore.subscribelistener mounted at App level.Closes #91 (T-223, Sprint 2 P1).
Why
Sprint 1 left no
settingstable — Sprint 2's project-tab persistence (issue spec) needs a backend home forui.selected_project_id. Generic KV here keeps door open for future UI prefs (kanban column widths, sidebar state, …) without revisiting the schema.Changes
Backend (Rust + libsql)
V002__settings.sql:settings (key TEXT PRIMARY KEY, value_json TEXT NOT NULL, updated_at INTEGER NOT NULL).domain/settings/{mod,model,ports,service}.rs:SettingsRepositorytrait (get/set),servicevalidating keys (non-empty, ≤256 chars, no control / bidi) and values (no NUL, no bidi, ≤64 KB),SELECTED_PROJECT_ID_KEYconstant mirrored to TS.infrastructure/persistence/settings_repo.rs:LibsqlSettingsRepoupsert viaINSERT … ON CONFLICT(key) DO UPDATE. Stablemap_db_err(ADR-016 — no libsql wording leaks; raw text →tracing::error!).application/di.rs: newsettings_repo: Arc<dyn SettingsRepository + Send + Sync>field onAppContainer, wired against the sameArc<Database>as the other repos.interfaces/tauri_commands/settings.rs: thinsettings_get(key)/settings_set(key, value)adapters; both registered alphabetically inregister.rs+ anchored inbin/specta-export.rs(use+anchor_commands+COMMANDS_TS).infrastructure/persistence/pool.rs: new V002 + history tests; existing single-migration assertion relaxed to[1, 2].Frontend (React 19 + Zustand)
src/shared/ipc/settings.ts:invokeSettingsGet/invokeSettingsSet.src/features/projects/persist-selected-project-id.ts:SELECTED_PROJECT_ID_KEY = "ui.selected_project_id"(mirrored against the Rust constant by an equality test);loadPersistedSelectedProjectId(): Promise<number | undefined>(rejects null / non-integer / non-positive);persistSelectedProjectId(id: number)fire-and-forget withconsole.warnon rejection.App.tsx: boot effect awaitsloadPersistedSelectedProjectId()AFTERsetProjects(soreconcileSelectedIdcannot drop a stale id before validation), restores when the persisted id matches a known project, falls back to first-project otherwise; seconduseEffectmountsuseProjectStore.subscribeso everyselectedIdchange persists without touchingProjectTabBaroruseProjectCreateForm.Acceptance criteria
embed_migrations!).settings_repo9 cases + DI parity test).App.test.tsxT-223 describe block, 6 cases).Test plan
cargo test --workspace— 274/274 pass.cargo clippy --workspace --all-targets -- -D warnings— 0 errors.cargo fmt --checkclean.cargo deny check— advisories / bans / licenses / sources OK.pnpm exec vitest run— 330/330 pass.pnpm exec tsc -b— clean.pnpm exec oxlint src/— 0 errors (22 warnings, baseline parity).pnpm exec oxfmt --check src/— clean.pnpm codegen—commands.tsregenerated with the two new entries (drift gate green).pnpm exec knip— no new unused exports.Notes for reviewers
App.tsxand not in the store: Zustand store stays infrastructure-free per ARCHI.md hexagonal rule; persistence is a side-effect that belongs at the composition boundary. SingleuseProjectStore.subscribelistener at App level avoids threading the side-effect through everyselect(id)call site (3 today: boot,ProjectTabBartab click,useProjectCreateForm.finalize).setProjects: the store'sreconcileSelectedIdclearsselectedIdif it's not in the freshly-loaded project list. Setting the persisted id BEFOREsetProjectswould race against this and silently drop the restore. The boot order is:setProjects(projects)→await loadPersistedSelectedProjectId()→ if persisted id matches a known project use it, else first project, else none.deleteonSettingsRepository: out-of-scope for Sprint 2 (issue T-223: Persist current_project_id across restarts (settings KV) #91 spec isget+set). Trivial to add when a future feature needs it.value_jsoncolumn name: the doc string + module comments document that values are opaque strings; callers JSON-encode if they need structure. The current writer (persistSelectedProjectId) stores a stringifiedi64.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation