Skip to content

Extract shared ID-existence/kind validation helper (reference targets + access-token grants) - #206

Merged
brylie merged 2 commits into
mainfrom
issue-62-shared-id-validation
Sep 4, 2026
Merged

Extract shared ID-existence/kind validation helper (reference targets + access-token grants)#206
brylie merged 2 commits into
mainfrom
issue-62-shared-id-validation

Conversation

@brylie

@brylie brylie commented Sep 4, 2026

Copy link
Copy Markdown
Owner

Summary

Three axes of the same duplication had grown independently across records.ts (create-vs-write, document-vs-collection target kind) and tokens.ts (existence-with-permission vs. existence-without-permission), per issue #62's history across PR #61/#193/#198.

  • src/lib/services/records.ts: validateDocumentReferenceTarget/validateCollectionReferenceTarget collapse into one validateReferenceTarget(caller, targetId, kind), built on resolveInternalLinkTarget (data/links.ts) instead of each calling crdtGetDocument/crdtGetCollection directly. The repeated "check parent is a Document, then validate the target" shape inside validateCreateReferencedRecordId/validateReferencedRecordIdWrite is now requireParentDocumentThenValidateTarget — the genuinely different parts (which block types each accepts, error wording) stay in each of them, since that difference is real (writeRecord doesn't support retargeting child_pages).
  • src/lib/services/tokens.ts: createToken's allowedDocumentIds/allowedCollectionIds now get the same existence check allowedSpaceIds already had, via one generic validateEvery(ids, existsFn, ErrorClass) helper shared across all three grant lists. The new Document/Collection checks build on resolveInternalLinkTarget — the same primitive the records.ts validators above now use.
  • Wires the two new error classes (UnknownDocumentError, UnknownCollectionError) through the /settings/tokens route handler, mirroring UnknownSpaceError's existing mapping.
  • Updates docs/specifications/service-layer.md and internal-links.md to describe the new shared validation path and the additional resolveInternalLinkTarget consumers.

Test plan

  • npm run test — 1003/1003 passing. All 997 pre-existing tests pass unmodified (no behavior change for any existing call site), plus 6 new tests covering the previously-unvalidated Document/Collection grant-existence checks (service-layer tokens.test.ts and route-level page.server.test.ts).
  • npm run lint — clean (prettier + eslint) for every file this PR touches.
  • npm run check — clean.

Closes #62

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Token creation now validates selected documents and collections, including internal-link targets and resource types.
    • Invalid or nonexistent document and collection selections return clear errors, and no token is created.
  • Bug Fixes

    • Improved record reference validation for document and collection targets.
    • Added consistent checks for token access scopes and referenced resource types.
  • Documentation

    • Updated service-layer and internal-link specifications to reflect the expanded validation behavior.

…ets and token grants

- src/lib/services/records.ts: validateDocumentReferenceTarget and
  validateCollectionReferenceTarget collapse into one
  validateReferenceTarget(caller, targetId, kind) built on
  resolveInternalLinkTarget (data/links.ts) instead of each calling
  crdtGetDocument/crdtGetCollection directly. The repeated "check parent
  is a Document, then validate the target" shape in
  validateCreateReferencedRecordId/validateReferencedRecordIdWrite now
  goes through requireParentDocumentThenValidateTarget; create-vs-write's
  genuinely different accepted block types and error wording stay in
  each of them.
- src/lib/services/tokens.ts: createToken's allowedDocumentIds/
  allowedCollectionIds now get the same existence check
  allowedSpaceIds already had, via one generic validateEvery(ids,
  existsFn, ErrorClass) helper shared across all three grant lists.
  Document/Collection existence checks build on resolveInternalLinkTarget,
  the same primitive the records.ts validators above now use.
- Wires the two new error classes (UnknownDocumentError,
  UnknownCollectionError) through the /settings/tokens route handler,
  mirroring UnknownSpaceError's existing mapping.

No behavior change for any existing call site (all 997 pre-existing
tests pass unmodified); adds regression coverage for the previously-
unvalidated Document/Collection grant-existence checks.

Closes #62

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@coderabbitai

coderabbitai Bot commented Sep 4, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

  • Run on-demand review

This review includes 1 billable file and costs up to $0.25.

Or wait 49 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 60 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Essentials

Run ID: efb88571-d20c-4be8-adbb-39f5b02bfd62

📥 Commits

Reviewing files that changed from the base of the PR and between c1ee150 and 3b765c5.

📒 Files selected for processing (1)
  • .agents/skills/pr-backlog-reflection/SKILL.md

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Essentials

Run ID: 7c73c24f-9a1d-41d8-a108-4e58699bf9d6

📥 Commits

Reviewing files that changed from the base of the PR and between 81ddca2 and c1ee150.

📒 Files selected for processing (7)
  • docs/specifications/internal-links.md
  • docs/specifications/service-layer.md
  • src/lib/services/records.ts
  • src/lib/services/tokens.test.ts
  • src/lib/services/tokens.ts
  • src/routes/settings/tokens/+page.server.ts
  • src/routes/settings/tokens/page.server.test.ts

Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour.


📝 Walkthrough

Walkthrough

Record references and token grants now use shared target and existence validation. Token creation rejects unknown or mismatched Document and Collection IDs before persistence, and the settings route returns specific HTTP 400 errors.

Changes

Reference and token validation

Layer / File(s) Summary
Shared record reference validation
src/lib/services/records.ts, docs/specifications/internal-links.md
Record creation and writes resolve referencedRecordId through resolveInternalLinkTarget, enforce Document or Collection kinds, and preserve access-token scope checks.
Token grant validation
src/lib/services/tokens.ts, src/lib/services/tokens.test.ts, docs/specifications/service-layer.md
createToken validates Space, Document, and Collection IDs with shared helpers. Typed errors cover unknown Documents and Collections.
Token route errors and coverage
src/routes/settings/tokens/+page.server.ts, src/routes/settings/tokens/page.server.test.ts
The settings route returns HTTP 400 errors for unknown Document or Collection selections. Tests verify that invalid submissions do not mint tokens.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to c1ee1

The shared validation changes preserve the intended grant and reference behavior without introducing a current merge-blocking risk.

Sequence Diagram(s)

sequenceDiagram
  participant TokenRoute
  participant createToken
  participant resolveInternalLinkTarget
  participant TokenStore
  TokenRoute->>createToken: submit selected resource IDs
  createToken->>resolveInternalLinkTarget: resolve Document and Collection IDs
  resolveInternalLinkTarget-->>createToken: return target kind or undefined
  createToken->>TokenStore: persist token after validation
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the primary change: extracting shared ID-existence and kind validation for reference targets and access-token grants.
Linked Issues check ✅ Passed The changes satisfy issue #62. They reuse resolveInternalLinkTarget for document and collection references, share record validation logic, add validateEvery for space, document, and collection grants,…
Out of Scope Changes check ✅ Passed The code, tests, route handling, and documentation changes directly support issue #62 objectives. No unrelated changes are identified.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 5 files. (2 skipped: 2 …
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-62-shared-id-validation

Comment @coderabbitai help to get the list of available commands.

.agents/skills/pr-backlog-reflection/SKILL.md (merged in PR #201) had a
malformed Markdown table that failed the repo-wide prettier --check gate,
breaking CI for any PR opened after it landed on main — including this
one. Whitespace-only reformat, no content change.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
brylie added a commit that referenced this pull request Sep 4, 2026
.agents/skills/pr-backlog-reflection/SKILL.md (merged in PR #201) has a
malformed Markdown table that fails the repo-wide prettier --check gate,
breaking npm run lint (and therefore CI) for every open PR — confirmed
on both #206 and #202. Whitespace-only reformat, no content change.

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
@brylie
brylie merged commit 671cdb7 into main Sep 4, 2026
2 checks passed
@brylie
brylie deleted the issue-62-shared-id-validation branch September 4, 2026 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extract shared ID-existence/kind validation helper (reference targets + access-token grants)

1 participant