Task-list-first personal task manager (template)#2018
Open
mpiniarski wants to merge 23 commits into
Open
Conversation
Import the task-list-first app as templates/tasks, register it as a hidden template for monorepo dev and CLI scaffolding, and trim framework boilerplate pages so the UI focuses on inbox, tasks, and fields. Co-authored-by: Cursor <[email protected]>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
Visual recap — skippedThe visual recap job did not run for this pull request. This is informational only and does not block the PR. Recap skipped for |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: Cursor <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Make bulk task/inbox mutations and mixed task+field updates transactional, declare the custom-field value unique index in schema, and reject duplicate field ids when reordering custom fields. Co-authored-by: Cursor <[email protected]>
Copy 37 drifted shadcn/ui components from the majority canonical templates so the ui-primitives sync guard passes in core CI.
Apply repository formatting to the tasks template so oxfmt --check passes in CI.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
mpiniarski
changed the title
Add tasks template raw UI strings to the i18n debt baseline and align templates/tasks/.agents/skills with canonical workspace skills so CI guards pass.
Wire @agent-native/toolkit for synced UI primitives, fix icon button sizes after the primitive sync, and list i18n as the first tasks README TODO. Co-authored-by: Cursor <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Fetch incomplete tasks by default with a hasCompletedTasks hint for the all-complete empty state, widen the query for completed deep links, and map ListCheck to check-square in the mobile app card icon table. Co-authored-by: Cursor <[email protected]>
When navigate omits includeDone, keep the current /tasks filter from the URL so agent-driven selection changes do not drop Show all. Co-authored-by: Cursor <[email protected]>
Contributor
There was a problem hiding this comment.
Builder reviewed your changes and found 5 potential issues 🟡
Review Details
I re-reviewed the latest PR head and re-checked the previously posted findings. The two already-open comments from the last incremental pass are still present, so I did not resolve them, and the earlier data-integrity findings from prior reviews also still appear unresolved. Overall this remains a high-risk PR because it introduces a large new action/navigation surface in a template app, and the branch is still not in a shippable state.
The new issues in this pass are concentrated around tab-scoped app state and bulk UI behavior. The Tasks app already carries a browser tab id through its client state machinery, but several newly added actions/hooks bypass the tab-scoped helpers and read/write global keys instead. In multi-tab usage, that can make chat-driven navigation and view-screen snapshots target the wrong tab or wrong selected rows. I also found one migration bug that can silently lose saved task-card-field preferences, plus a bulk toolbar path that still partially applies changes even when the UI reports failure.
Good news: I did not find a new auth bypass in the updated code, and browser testing was attempted but remains unavailable in this environment.
🧪 Browser testing: Skipped — infra-unavailable. The dev server is up, but the hidden Tasks template still is not reachable for preview (/tasks returns 404 and port 8091 is not serving).
Scope navigate, view-screen, and list selection to the requesting browser tab, harden legacy task-card field migration, and route bulk complete and mark-ready toolbar actions through transactional server batches. Co-authored-by: Cursor <[email protected]>
Contributor
|
Thanks for putting this PR together — this is a substantial addition, and overall it looks really solid. The architecture is thoughtful, ownership scoping looks good, and the test coverage is strong. Nice work addressing the earlier review findings too. A couple things I’d fix before merging:
A few smaller follow-ups:
Once those are addressed, this looks good to me. |
Prevalidate ownership and delete custom-field values and the task row in one transaction so a failed delete cannot orphan field data. Co-authored-by: Cursor <[email protected]>
Bulk deletes/updates dedupe repeated ids so counts and returned rows reflect real work; reorders reject them, since an ordering must name every visible item exactly once. Neither path had tests. Adds regression coverage for repeated ids in bulkDeleteTasks, bulkUpdateTasks, bulkDeleteInboxItems, and bulkMarkInboxItemsReady, plus duplicate rejection in reorderTasks and reorderInboxItems. The reorder case previously slipped past validation when the id count still matched the visible-list size, silently collapsing two rows onto one sort position.
Replace the chat-starter marketing copy inherited from the chat template with copy about the inbox, task list, custom fields, and agent parity. Drop the dead {{APP_TITLE}} placeholder check and its "Chat" fallback.
navigate accepted an arbitrary path and passed it to the router. Drop the path parameter and make view a required enum, so every target is one the app defines. Accept home/ask as aliases, resolved to a canonical view before the application-state write. Fix the AGENTS.md action table, which advertised a chat view that does not exist.
The bulk and reorder actions accepted unbounded id arrays: every one used .min(1) with no upper limit, so a single call could pass an arbitrarily long list straight through to the store layer. Add a shared BULK_ID_LIMIT of 500 and apply it to all seven id arrays. The value sits under SQLite's bind-parameter ceiling while staying well above a realistic list. Note this is a real behavior boundary, not just a safety net: reorder requires every visible id, and the UI's select-all sends every selected id, so a list longer than 500 can no longer be reordered or bulk-acted on in one call.
The store issued one statement per id on every bulk path: a SELECT per id to check existence, an UPDATE or DELETE per id to apply the change, and a further SELECT per id to read results back. Reorder and select-option cleanup were worse — they rewrite every row an owner has, which BULK_ID_LIMIT does not cap. Replace the per-id loops with batch statements. Uniform patches use one inArray UPDATE/DELETE; reads use one inArray SELECT. Where each row needs a different value (sort order, trimmed select values), a chunked CASE <id> WHEN … END expression writes them all in one statement per chunk, sized to stay under SQLite's bind-parameter ceiling. Also fixes a latent bug this exposed: cleanupValuesAfterConfigChange parsed stored values against the *new* field config, and parseStoredValue throws on a now-invalid option — so removing an in-use select option threw "Select value is not a valid option." and rolled back the whole update. Cleanup now reads the raw stored shape instead of validating it against the config that just invalidated it. For a list of N: bulk update 3N+ statements to 3, bulk delete 2N+ to 3, reorder N to ceil(N/200), select cleanup up to 2N to 2.
The stores were synchronous and used .run(), .all() and .get(), which are sqlite-core only: absent on Postgres, and unawaited promises on libsql. They only ever worked on better-sqlite3. Replace the custom sync runTransaction helper with drizzle's db.transaction(async (tx) => ...), await the query builders, and give the transaction handle a real type instead of `any`. The `any` collapsed the db|tx union, which is why the SQLite-only calls typechecked at all. The sync design came from the test harness rather than from production: tests mocked the db with a raw better-sqlite3 instance, whose native transaction wrapper rejects async callbacks. Tests now run on libsql, the same async driver the hosted app uses, so sync-only calls fail there instead of in production. No behaviour change: the suite reports the same 5 failures and 124 passes as before. Those failures are pre-existing -- the duplicate-id tests added in f04cab4 still have no implementation.
Both branches rewrote the same store files. task-template made the store driver-portable (async, no better-sqlite3-only .run()/.all(), DbHandle in place of the sync transaction helper); this branch replaced the per-id statement loops with batched ones. Resolved by keeping the portable async structure and re-expressing the batching inside it: the inArray existence check, listStoredItemsByIds, the plural *StoredItemsInTx primitives, the chunked CASE writes for reorder/promote/select cleanup, and the multi-row upsert are all async now and go through DbHandle. No sync driver calls survive.
Store functions were split between two conventions: some took a required `tx` as their first argument, most took no handle at all and reached for `getDb()` internally. A function in the second group could not be composed into a caller's transaction, so a read inside a transaction silently ran on `getDb()` instead -- on Postgres a different pooled connection, which cannot see the transaction's uncommitted writes and can block on its locks. All 52 store functions now take `db: DbHandle = getDb()` as an optional trailing argument and open transactions on that handle rather than on `getDb()`. Called bare a function opens its own transaction; called with a `tx` it joins the caller's, which drizzle turns into a SAVEPOINT, so the work is atomic either way and no function needs to know which situation it is in. DbHandle gains "transaction" and stays a structural Pick: the db type has batch() and a transaction does not, so a transaction is not assignable to the db type, and deriving from getDb keeps it dialect-agnostic. Note the handle deliberately does not distinguish a transaction from the plain db, so functions must never branch on whether one was passed -- passing getDb() to an `if (db) use it; else open a transaction` would skip the transaction and silently lose atomicity. Documented in AGENTS.md along with the other rules. No behaviour change: 131 tests pass, as before.
Both chunk-boundary tests created 250 tasks one at a time to cross the 200-row write chunk, ~2000 serial statements each, and timed out under parallel suite load. Mock the chunk size to 2 so they cross the boundary with 5 tasks, and cover the real constant's 999-parameter budget with a direct unit test.
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
2 participants
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.
Tasks
A task-list-first personal task manager. Capture rough ideas in an inbox, triage them, then promote the ones worth doing into tasks.
Features
Task list (
/tasks)Inbox (
/inbox)Custom fields (
/fields)Agent sidebar
view-screen(current list, selection, filters)Coming later
Projects, saved views, search, and richer onboarding — the template ships the core list + inbox + fields MVP.
One line: Capture in the inbox, triage with mark ready, run your day from
/tasks, extend tasks with custom fields.Test plan
pnpm --filter tasks testpnpm --filter tasks typecheckpnpm --filter tasks test:e2e