|
| 1 | +# Plan: Deduplicate week-navigation logic (R3) — #200 |
| 2 | + |
| 3 | +**Reference spec:** [docs/specs/2026-05-17-200-refactor-week-nav-dedup.md](../specs/2026-05-17-200-refactor-week-nav-dedup.md) |
| 4 | + |
| 5 | +## Approach |
| 6 | + |
| 7 | +Surgical extraction: move the 6-line block into a new exported function and replace both call sites. No behaviour change. All existing tests are regression guards — they run unchanged and must stay green. New unit tests cover the extracted function directly, without going through the full command layer. |
| 8 | + |
| 9 | +Trade-off: importing `Input` from `'../model'` in `navigation.ts` adds a domain→model dependency that wasn't there before. This is acceptable; `navigation.ts` already returns `Date` objects and `Anchor` structs that the command layer converts to `Input`. Returning an `Input` directly shortens the command glue to two lines. |
| 10 | + |
| 11 | +## Steps |
| 12 | + |
| 13 | +### 1 — Add imports to `src/actions/navigation.ts` |
| 14 | + |
| 15 | +Add at top of file: |
| 16 | + |
| 17 | +```typescript |
| 18 | +import { getWeekFromURIAndConfig } from '../util/paths'; |
| 19 | +import { Input } from '../model'; |
| 20 | +import moment = require("moment"); |
| 21 | +``` |
| 22 | + |
| 23 | +**Why:** the new function delegates to `getWeekFromURIAndConfig` (already used by the two command files) and returns an `Input` so callers need zero additional allocation. |
| 24 | + |
| 25 | +### 2 — Add `getAdjacentWeekInput` to `src/actions/navigation.ts` |
| 26 | + |
| 27 | +Export the following function (full body in spec): |
| 28 | + |
| 29 | +```typescript |
| 30 | +export async function getAdjacentWeekInput( |
| 31 | + editor: vscode.TextEditor | undefined, |
| 32 | + ctrl: Ctrl, |
| 33 | + direction: Direction |
| 34 | +): Promise<Input | undefined> |
| 35 | +``` |
| 36 | + |
| 37 | +Implementation detail: |
| 38 | +- Guard: if no editor → return `undefined`. |
| 39 | +- Call `getWeekFromURIAndConfig(editor.document.uri, ctrl.config)`. |
| 40 | +- If no match → return `undefined` (caller falls through to daily-entry navigation). |
| 41 | +- Compute adjacent week with `moment().week(weekInfo.week).weekYear(weekInfo.year)[direction === 'previous' ? 'subtract' : 'add'](1, 'week')`. |
| 42 | +- Set `input.week = adj.week()` and return. |
| 43 | + |
| 44 | +**Why:** ternary on `direction` eliminates the only difference between the two duplicated blocks. |
| 45 | + |
| 46 | +### 3 — Refactor `src/provider/commands/open-previous-entry.ts` |
| 47 | + |
| 48 | +- Remove `import { getWeekFromURIAndConfig } from '../../util/paths'`. |
| 49 | +- Remove `import moment = require("moment")`. |
| 50 | +- Replace the duplicated 6-line block with: |
| 51 | + |
| 52 | +```typescript |
| 53 | +const weekInput = await getAdjacentWeekInput(editor, this.ctrl, 'previous'); |
| 54 | +if (weekInput) { |
| 55 | + await this.execute(weekInput); |
| 56 | + return; |
| 57 | +} |
| 58 | +``` |
| 59 | + |
| 60 | +Add `getAdjacentWeekInput` to the import from `'../../actions/navigation'`. |
| 61 | + |
| 62 | +**Why:** removes the dead imports, leaves command as pure glue. |
| 63 | + |
| 64 | +### 4 — Refactor `src/provider/commands/open-next-entry.ts` |
| 65 | + |
| 66 | +Identical to Step 3, but `direction` = `'next'`. |
| 67 | + |
| 68 | +### 5 — Add unit tests |
| 69 | + |
| 70 | +Add a new `suite('getAdjacentWeekInput')` block in `src/test/suite/commands-prev-next.test.ts`, under the existing `helper layer with seeded base` suite. Scenarios: |
| 71 | + |
| 72 | +| # | Name | Setup | Expected | |
| 73 | +|---|------|-------|----------| |
| 74 | +| T1 | no editor | `editor = undefined` | returns `undefined` | |
| 75 | +| T2 | non-weekly file | open a day entry `${tmpBase}/2026/05/16.md` as active | returns `undefined` | |
| 76 | +| T3 | weekly file + next | create and open `${tmpBase}/2026/w20.md`; call with `'next'` | `input.week === 21` | |
| 77 | +| T4 | weekly file + previous | same weekly file; call with `'previous'` | `input.week === 19` | |
| 78 | + |
| 79 | +Seed weekly files using the same `vscode.workspace.fs.createDirectory` + `writeFile` pattern as `seedEntry`. Weekly file naming follows the default pattern: `${year}/w${week}.md`. |
| 80 | + |
| 81 | +**Why:** tests target the pure extracted logic, not the full command stack — fast and focused. |
| 82 | + |
| 83 | +### 6 — Compile and test |
| 84 | + |
| 85 | +```bash |
| 86 | +npm run compile-tests && npm test |
| 87 | +``` |
| 88 | + |
| 89 | +Full `npm run check` must pass (lint + compile + test). |
| 90 | + |
| 91 | +## Test scenarios summary |
| 92 | + |
| 93 | +- **T1–T2** (non-weekly): guard conditions; confirm function does not return an Input for inapplicable files. |
| 94 | +- **T3–T4** (weekly): confirm direction-to-add/subtract mapping and that the returned `Input.week` is correct. |
| 95 | +- **Regression**: all 16 existing navigation tests in `commands-prev-next.test.ts` must pass unchanged — they cover the command-layer behaviour end-to-end. |
| 96 | + |
| 97 | +## Dependencies |
| 98 | + |
| 99 | +None. Work is self-contained within this repo and does not require any other PR to land first. |
| 100 | + |
| 101 | +## Risk |
| 102 | + |
| 103 | +**Week-boundary crossing (week 52/53 → week 1):** pre-existing moment behaviour, not changed by this refactoring. Covered by the fact that moment's `.weekYear()` handles it correctly; no test added here since behaviour is unchanged. |
| 104 | + |
| 105 | +**Import cycle:** `navigation.ts` → `../model` → no back-edge into `navigation.ts`. No cycle introduced. |
| 106 | + |
| 107 | +## Rollback |
| 108 | + |
| 109 | +`git revert <commit>` on the single implementation commit. No data migrations, no schema changes. |
0 commit comments