max / goingson
- Co-Authored-By
- Claude Opus 5 (1M context) <noreply@anthropic.com>
4 files changed,
+36 insertions,
-26 deletions
| @@ -203,14 +203,23 @@ | |||
| 203 | 203 | return html; | |
| 204 | 204 | } | |
| 205 | 205 | ||
| 206 | + | // The icon says what the goal is; the button's label says what pressing it will | |
| 207 | + | // do, and the move it names is the one it sends. A control that derives its | |
| 208 | + | // target from the status it was drawn with races a second window that has | |
| 209 | + | // already moved the goal. | |
| 210 | + | const GOAL_MOVES = { | |
| 211 | + | active: { icon: '○', next: 'done', label: 'Mark done' }, | |
| 212 | + | done: { icon: '✓', next: 'abandoned', label: 'Give up on it' }, | |
| 213 | + | abandoned: { icon: '✗', next: 'active', label: 'Make it active again' }, | |
| 214 | + | }; | |
| 215 | + | ||
| 206 | 216 | function renderGoalItem(goal, position) { | |
| 207 | - | const statusIcons = { active: '○', done: '✓', abandoned: '✗' }; | |
| 208 | - | const icon = statusIcons[goal.status] || '○'; | |
| 217 | + | const move = GOAL_MOVES[goal.status] || GOAL_MOVES.active; | |
| 209 | 218 | ||
| 210 | 219 | let html = `<div class="row scope-slot month-goal-item filled ${goal.status}">`; | |
| 211 | 220 | html += `<span class="scope-slot-label">Goal #${position}</span>`; | |
| 212 | 221 | html += `<div class="month-goal-body">`; | |
| 213 | - | html += `<button class="button--icon month-goal-status-btn" data-act="monthlyReview.cycleGoalStatus" data-a1="${escAttr(goal.id)}" title="Cycle status">${icon}</button>`; | |
| 222 | + | html += `<button class="button--icon month-goal-status-btn" data-act="monthlyReview.setGoalStatus" data-a1="${escAttr(goal.id)}" data-a2="${escAttr(move.next)}" title="${escAttr(move.label)}" aria-label="${escAttr(move.label)}">${move.icon}</button>`; | |
| 214 | 223 | html += `<span class="scope-slot-title month-goal-text">${esc(goal.text)}</span>`; | |
| 215 | 224 | html += `<button class="button--icon row-actions month-goal-delete-btn" data-act="monthlyReview.deleteGoal" data-a1="${escAttr(goal.id)}" title="Delete goal">✕</button>`; | |
| 216 | 225 | html += `</div>`; |
| @@ -201,21 +201,19 @@ | |||
| 201 | 201 | } | |
| 202 | 202 | ||
| 203 | 203 | /** | |
| 204 | - | * Cycle a goal's status: active -> done -> abandoned -> active. | |
| 204 | + | * Set a goal's status to a named target. | |
| 205 | + | * | |
| 206 | + | * The target comes from the button that was pressed, not from a copy of the | |
| 207 | + | * goal read at render time: a window that computed the next status from what it | |
| 208 | + | * last saw would write a status derived from a stale reading whenever a second | |
| 209 | + | * window had already moved the goal. | |
| 210 | + | * | |
| 205 | 211 | * @param {string} id - Goal ID | |
| 212 | + | * @param {string} status - Target status: active, done or abandoned | |
| 206 | 213 | */ | |
| 207 | - | async function cycleGoalStatus(id) { | |
| 208 | - | const data = GoingsOn.state.monthlyReview; | |
| 209 | - | if (!data) return; | |
| 210 | - | ||
| 211 | - | const goal = data.goals.find(g => g.id === id); | |
| 212 | - | if (!goal) return; | |
| 213 | - | ||
| 214 | - | const next = { active: 'done', done: 'abandoned', abandoned: 'active' }; | |
| 215 | - | const newStatus = next[goal.status] || 'active'; | |
| 216 | - | ||
| 214 | + | async function setGoalStatus(id, status) { | |
| 217 | 215 | await GoingsOn.ui.apiCall( | |
| 218 | - | GoingsOn.api.monthlyReview.updateGoalStatus(id, newStatus), | |
| 216 | + | GoingsOn.api.monthlyReview.updateGoalStatus(id, status), | |
| 219 | 217 | { reload: load } | |
| 220 | 218 | ); | |
| 221 | 219 | } | |
| @@ -337,7 +335,7 @@ | |||
| 337 | 335 | nextMonth, | |
| 338 | 336 | goToCurrentMonth, | |
| 339 | 337 | addGoal, | |
| 340 | - | cycleGoalStatus, | |
| 338 | + | setGoalStatus, | |
| 341 | 339 | deleteGoal, | |
| 342 | 340 | navigateToDay, | |
| 343 | 341 | showDaySummary, |
| @@ -325,20 +325,23 @@ | |||
| 325 | 325 | /// **A control that cycles hidden state cannot be described, and should not | |
| 326 | 326 | /// be.** | |
| 327 | 327 | /// | |
| 328 | - | /// `monthly-review.js:cycleGoalStatus` reads the goal out of module state, | |
| 329 | - | /// looks up `active -> done -> abandoned -> active`, and writes the next one. | |
| 330 | - | /// Two things are wrong with it and only one is the description layer's. | |
| 328 | + | /// `monthly-review.js:cycleGoalStatus` read the goal out of module state, | |
| 329 | + | /// looked up `active -> done -> abandoned -> active`, and wrote the next one. | |
| 330 | + | /// Two things were wrong with it and only one was the description layer's. | |
| 331 | 331 | /// | |
| 332 | 332 | /// The describable half: a button labelled with the *current* status, whose | |
| 333 | 333 | /// effect is a table the user cannot see, says nothing about what pressing it | |
| 334 | 334 | /// will do. Here each goal offers the move by name — "Mark done", "Give up on | |
| 335 | 335 | /// it", "Make it active again" — so the label is the outcome. | |
| 336 | 336 | /// | |
| 337 | - | /// The half that is a real defect in the shipped screen: computing the next | |
| 337 | + | /// The half that was a real defect in the shipped screen: computing the next | |
| 338 | 338 | /// status from a copy read at render time races a second window, which will | |
| 339 | 339 | /// write a status derived from what it saw rather than from what is stored. | |
| 340 | 340 | /// Naming the target explicitly removes the race as a side effect, because the | |
| 341 | 341 | /// route no longer has to know what the goal was before. | |
| 342 | + | /// | |
| 343 | + | /// Both halves were ported back into the JS afterwards: `setGoalStatus` takes | |
| 344 | + | /// the target, and the button carries the move as its label. | |
| 342 | 345 | fn goals(data: &MonthlyReviewData, month: NaiveDate) -> Vec<Node> { | |
| 343 | 346 | let mut out = vec![Node::section("Goals")]; | |
| 344 | 347 |
| @@ -232,10 +232,10 @@ | |||
| 232 | 232 | ||
| 233 | 233 | #[tokio::test] | |
| 234 | 234 | async fn a_goal_names_the_move_it_offers_rather_than_the_state_it_is_in() { | |
| 235 | - | // The second finding. `cycleGoalStatus` reads the goal from module state, | |
| 236 | - | // looks the next status up in a table the user cannot see, and writes it. | |
| 237 | - | // Described, each goal offers the move by name, so the label is the | |
| 238 | - | // outcome. | |
| 235 | + | // The second finding. The JS used to read the goal from module state, look | |
| 236 | + | // the next status up in a table the user cannot see, and write it. Described, | |
| 237 | + | // each goal offers the move by name, so the label is the outcome; the JS was | |
| 238 | + | // ported onto the same shape afterwards (`setGoalStatus`). | |
| 239 | 239 | let state = state().await; | |
| 240 | 240 | add_goal(&state, "Ship the thing"); | |
| 241 | 241 | ||
| @@ -259,8 +259,8 @@ | |||
| 259 | 259 | #[tokio::test] | |
| 260 | 260 | async fn the_target_status_is_named_so_two_windows_cannot_race() { | |
| 261 | 261 | // The half of the second finding that is a real defect rather than a | |
| 262 | - | // description gap: the JS derives the next status from a copy read at | |
| 263 | - | // render time, so a second window writes a status derived from what it saw. | |
| 262 | + | // description gap: the JS derived the next status from a copy read at | |
| 263 | + | // render time, so a second window wrote a status derived from what it saw. | |
| 264 | 264 | // The route takes the target explicitly, so a stale screen cannot invent | |
| 265 | 265 | // one. | |
| 266 | 266 | let state = state().await; |