Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bde642194f
ℹ️ 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".
| output.push(line); | ||
| } | ||
| writeFileSync(taskFile, output.join("\n")); | ||
| addFrontmatterField(taskFile, "priority", newPriority); |
There was a problem hiding this comment.
Scope priority-field check to frontmatter before promote write
Calling addFrontmatterField(taskFile, "priority", newPriority) here can silently fail to persist a promotion when the task has no priority in frontmatter but the markdown body contains a top-level priority: line. addFrontmatterField checks existence with content.includes("\npriority:") (in src/tasks/markdown.ts) and may choose updateFrontmatterField, which only edits inside frontmatter and therefore performs no insertion; the API still returns the new priority, so UI/ordering can diverge from the file state.
Useful? React with 👍 / 👎.
…ion, and frontmatter writing - Add `TASK_ID_RE` constant and `priorityValue()` export to `src/tasks/markdown.ts` - Remove dead local `priorityValue()` in `src/flow.ts` (was defined but never called) - Replace local `priorityValue()` in `src/dashboard.ts` with import from `tasks/markdown.ts` - Extract `resolveTaskFile()` helper in `dashboard-server.ts` to centralize path resolution + safety checks - Replace all four inline task ID regexes in `dashboard-server.ts` with `TASK_ID_RE` - Replace hand-rolled frontmatter loops in `/api/slot-postpone` and `/api/task-promote` with `updateFrontmatterField` / `addFrontmatterField` - Add unit tests for `priorityValue()` sorting (S < A < B < C, unknown → 9) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The previous `content.includes("\nfield:")` check scanned the entire file.
A matching line in the markdown body would cause addFrontmatterField to call
updateFrontmatterField, which is frontmatter-scoped and would silently no-op,
leaving the field unwritten while the API returns the new value.
Fix: extract frontmatter block and check only that content for the field,
preventing false-positive matches from the document body.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
84a18b4 to
e018874
Compare
Retrospective: What I'd Do Differently Next Timetask-010fa0f1 — Dashboard API cleanup: extract shared helpers1. Verify call sites before deleting a "duplicate" functionBefore removing 2. Audit utility functions for whole-file scans before delegating to them
Checklist for future utility delegation:
3. Add a regression test for the body-scope bug when fixing itThe test("addFrontmatterField ignores body lines when checking existence", () => {
// file has no frontmatter priority: field, but body contains "priority: foo"
// addFrontmatterField should INSERT into frontmatter, not silently no-op
});4. The proposal's
|
Summary
Follow-up to PR #79 — extracts duplicated logic from
dashboard-server.tsinto shared helpers to prevent a class of copy-paste bugs caught in the retrospective.TASK_ID_RE— exported fromsrc/tasks/markdown.ts, replaces four inline regexes acrossdashboard-server.tsresolveTaskFile(taskId)— local helper insidestartDashboardServerthat centralises the resolve + safety-check + existence-check pattern used bytask-promote,task-confirm,task-dismissupdateFrontmatterField/addFrontmatterField— existing helpers fromtasks/markdown.tsnow used in both/api/slot-postponeand/api/task-promote, replacing two hand-rolled 15-line frontmatter loopspriorityValue()— exported fromsrc/tasks/markdown.ts, imported intodashboard.ts; dead copy inflow.tsremovedsrc/tasks/priority-value.test.tstests S < A < B < C ordering and unknown → 9Test plan
bun run typecheck— cleanbun test— 257 pass, 0 fail (includes 4 newpriorityValuetests)🤖 Generated with Claude Code