Repository navigation
Make BoardRepository board-scoped - #3
Merged
Merged
Conversation
Step 1 of supporting multiple boards. The repository held a single board
under one key; it now addresses boards by id and can enumerate them, which
is what a board picker needs and what a server-backed implementation will
answer with a query.
list() -> [{ id, title, updatedAt }], newest first
load(id) -> record | null
save(id, board, { title }) -> boolean
rename(id, title) -> boolean
remove(id) -> boolean
migrateLegacy({ toId }) -> boolean
Boards are stored as an envelope — id, title, updatedAt, board — because a
picker needs a label and an ordering, and neither belongs in the object
graph. updatedAt comes from an injected `now`, so it stays deterministic in
tests, matching how timing is handled everywhere else.
createApp gains `boardId` (defaulting to 'default'), `namespace` and `now`,
replacing `storageKey`. Behaviour with one board is unchanged, so this is
inert until a board picker exists.
Two deliberate behaviour changes:
- An empty board now restores as empty. Previously `load()` treated a board
with no objects as "nothing saved" and re-seeded the starter board, so
clearing your canvas was silently undone by a refresh.
- Boards written by the single-board version are adopted on first run and
the old key is cleared, so the deployed site does not lose anyone's work.
257 tests, 99.8% line coverage.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe application now stores versioned board records by namespace and board ID. Startup migrates legacy data and restores the selected record. Autosave persists the selected board ID. Tests cover storage operations, isolation, migration, polling, and dynamic server ports. ChangesMulti-board persistence
Test execution support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant createApp
participant repository
participant autosave
participant localStorage
createApp->>repository: migrateLegacy to boardId
createApp->>repository: load boardId
repository-->>createApp: board record and title
createApp->>autosave: configure boardId
autosave->>repository: save boardId and board
repository->>localStorage: persist board record
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (3)
test/node/storage.test.js (1)
247-251: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that a failed copy leaves the legacy record in place.
This test proves the return value only. The property that matters is that no data is lost: if the copy cannot be written,
peerthink:boardmust still hold the original board.💚 Proposed addition
test('reports failure when the copy cannot be written', () => { storage.data.set(LEGACY_KEY, JSON.stringify(legacy)); storage.failOn = 'set'; assert.equal(repo.migrateLegacy(), false); + + storage.failOn = null; + assert.equal(storage.data.get(LEGACY_KEY), JSON.stringify(legacy), 'nothing is lost'); + assert.equal(repo.load(DEFAULT_BOARD_ID), null); });🤖 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 `@test/node/storage.test.js` around lines 247 - 251, Update the test case for migrateLegacy when storage.failOn is 'set' to also assert that storage.data still contains the original legacy record under LEGACY_KEY after migration fails, while preserving the existing false return-value assertion.test/browser/app.test.js (1)
120-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the
localStoragecleanup so a failed assertion does not leak keys.Both tests write real keys into the page's
localStorage, then remove them on the last line. If an assertion fails first, the removal never runs.peerthink:board:second-boardorscratch:board:defaultthen survives into later tests in the same page, andrepository.list()in other tests sees an extra board.
withSecondAppalready has afinallyblock forsecond.destroy(). Put the key removal in atry/finallyin the test bodies, or extendwithSecondAppto clear the keys it created.♻️ Proposed change for the board-id test; apply the same shape at lines 142-157
test('a board id keeps two boards in the same storage apart', async () => { - const result = await withSecondApp( - `storage: window.localStorage, boardId: 'second-board', autosaveDelay: 1`, - ... - ); - - assert.equal(result.boardId, 'second-board'); - assert.equal(result.written, true); - assert.equal(result.leaked, false, "the page's own board is untouched"); - assert.equal(result.listed, true, 'and it shows up in the board list'); - await page.eval(`localStorage.removeItem('peerthink:board:second-board')`); + try { + const result = await withSecondApp( + `storage: window.localStorage, boardId: 'second-board', autosaveDelay: 1`, + ... + ); + + assert.equal(result.boardId, 'second-board'); + assert.equal(result.written, true); + assert.equal(result.leaked, false, "the page's own board is untouched"); + assert.equal(result.listed, true, 'and it shows up in the board list'); + } finally { + await page.eval(`localStorage.removeItem('peerthink:board:second-board')`); + } });🤖 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 `@test/browser/app.test.js` around lines 120 - 157, Move localStorage cleanup for both tests into guaranteed finally blocks: ensure peerthink:board:second-board is removed in the board-id test and scratch:board:default in the namespace test even when assertions fail. Preserve the existing assertions and withSecondApp lifecycle while applying the same cleanup shape to both test bodies.test/browser/main.test.js (1)
6-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport the key constants instead of restating them.
public/js/platform/storage.jsexportsLEGACY_KEY.test/node/storage.test.jsimports it. This file redeclares bothLEGACY_KEYand the default board key as literals. If the namespace or the key layout changes, this suite keeps asserting the old strings and still passes.
KEYhas no exported counterpart today. Build it from the exported pieces so it tracks the source.♻️ Proposed change
-const LEGACY_KEY = 'peerthink:board'; -const KEY = 'peerthink:board:default'; +import { DEFAULT_BOARD_ID, LEGACY_KEY } from '../../public/js/platform/storage.js'; + +const KEY = `peerthink:board:${DEFAULT_BOARD_ID}`;🤖 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 `@test/browser/main.test.js` around lines 6 - 11, Update the key declarations in the browser storage tests to import LEGACY_KEY from public/js/platform/storage.js and construct the default board KEY from exported storage-key pieces rather than hardcoded literals. Preserve the existing record helper and ensure both constants stay synchronized with the production namespace and key layout.
🤖 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 `@public/js/platform/storage.js`:
- Around line 115-133: Update migrateLegacy to adopt and remove LEGACY_KEY only
when the repository is using the default namespace; non-default namespaces must
return without consuming the shared legacy board. Preserve the existing
migration, save, and cleanup behavior for the owning namespace.
- Around line 70-76: Update storage.list() to derive each returned record’s id
from the storage key used by keyFor(id), rather than trusting the parsed
record.id. Ensure updatedAt has a valid fallback before the sort comparator
runs, while preserving the existing filtering and projected result shape.
In `@test/browser/main.test.js`:
- Around line 68-76: Replace the fixed page.sleep(600) in the “it saves under
the board id, not a single global key” test with the existing
app.autosave.flush() readiness call before asserting localStorage contents.
Preserve the board ID assertion and both storage-key assertions.
---
Nitpick comments:
In `@test/browser/app.test.js`:
- Around line 120-157: Move localStorage cleanup for both tests into guaranteed
finally blocks: ensure peerthink:board:second-board is removed in the board-id
test and scratch:board:default in the namespace test even when assertions fail.
Preserve the existing assertions and withSecondApp lifecycle while applying the
same cleanup shape to both test bodies.
In `@test/browser/main.test.js`:
- Around line 6-11: Update the key declarations in the browser storage tests to
import LEGACY_KEY from public/js/platform/storage.js and construct the default
board KEY from exported storage-key pieces rather than hardcoded literals.
Preserve the existing record helper and ensure both constants stay synchronized
with the production namespace and key layout.
In `@test/node/storage.test.js`:
- Around line 247-251: Update the test case for migrateLegacy when
storage.failOn is 'set' to also assert that storage.data still contains the
original legacy record under LEGACY_KEY after migration fails, while preserving
the existing false return-value assertion.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d12eca1b-9ed2-4e61-b03d-79596c92edf1
📒 Files selected for processing (7)
public/js/app.jspublic/js/core/autosave.jspublic/js/platform/storage.jstest/browser/app.test.jstest/browser/main.test.jstest/node/autosave.test.jstest/node/storage.test.js
list() took id and updatedAt from the parsed record, but read() only validated `board`. A record written by an older version or edited by hand could carry a stale id — producing a picker entry whose load() returns null — or no updatedAt at all, which makes the comparator return NaN and scrambles the order of the whole list rather than just that row. The key is the record's real address, so it is now the source of the id, and both metadata fields have a floor. migrateLegacy read and deleted LEGACY_KEY regardless of namespace. A repository on `scratch` sharing one Web Storage would adopt the default workspace's legacy board and then delete it, so the workspace that actually owned it had nothing left to migrate. Adoption is now limited to the owning namespace, with an explicit `legacyKey` for callers that mean it. Two browser tests slept 600ms past a 400ms debounce, which is generous on a laptop and a coin flip on a loaded runner. They now poll a real condition via page.waitFor; the third asserts that nothing was written, where there is no positive condition to wait for, so it flushes the autosave instead. Also took the fixed port out of the server spawn test — a leftover process from an earlier run made it fail with a ten-second timeout that looked nothing like the actual cause. 261 tests, 99.8% line coverage.
This was referenced Aug 4, 2026
This was referenced Aug 4, 2026
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
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.
Step 1 of supporting multiple boards.
The repository held a single board under one key. It now addresses boards by id and can enumerate them — which is what a board picker needs, and what a server-backed implementation will answer with a query rather than a scan.
Boards are stored as an envelope —
{ id, title, updatedAt, board }— because a picker needs a label and an ordering, and neither belongs in the object graph.updatedAtcomes from an injectednow, so it stays deterministic in tests, matching how timing is handled everywhere else.createAppgainsboardId(defaulting to'default'),namespaceandnow, replacingstorageKey. Behaviour with a single board is unchanged, so this is inert until a board picker exists — which is the point of landing it separately.Two deliberate behaviour changes
An empty board now restores as empty. Previously
load()treated a board with no objects as "nothing saved" and re-seeded the starter board, so clearing your canvas was silently undone by a refresh. That was a bug; board-scoping forced the question. There is a test naming it explicitly.Boards written by the single-board version are adopted. Anything under the old
peerthink:boardkey migrates todefaulton first run and the old key is cleared, so the deployed site does not drop anyone's work. It refuses to overwrite an existing board, and leaves the old key alone if the copy cannot be written.Tests
257 tests, 99.8% line coverage.
src/platform/storage.jsis at 100%, including every failure mode — unreadable storage, corrupt JSON, full quota, and storage that cannot be enumerated.Summary by CodeRabbit
New Features
Bug Fixes