Rename Attribute concept to Record throughout codebase#174
Open
wolfoo2931 wants to merge 10 commits into
Open
Conversation
Renames src/attributes/ → src/records/, all Attribute classes to Record
equivalents (KeyValueRecord, LongTextRecord, BlobRecord, RecordsRepository),
and adds /records/* HTTP endpoints alongside the existing /attributes/* routes.
Backwards compatibility is preserved: client.Attribute getter, old class name
aliases (KeyValueAttribute etc.), old HTTP routes, and wire protocol strings
('KeyValueAttribute' in $hasDataType queries) all remain unchanged.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 45 minutes and 30 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR migrates the codebase and docs from an "attributes" surface to a "records" surface: renaming classes/exports, updating imports, REST endpoints, storage APIs, server routes, tests, and docs. Backwards-compatible aliases are added; core control flow remains unchanged. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/controllers/records_controller.ts (1)
178-191:⚠️ Potential issue | 🟡 MinorLog messages still hardcode
/attributes/...even when invoked via/records.With both
/attributes/:attributeIdand/records/:attributeIdmounted on this controller (seesrc/server/routes.ts:36-47), the literal path in these error logs will be wrong for half the traffic, making debugging harder. Consider deriving the path from the request:♻️ Suggested fix
- if (result instanceof Error) { - req.log.error(`error in POST /attributes/${req.params.attributeId}`, result.message); + if (result instanceof Error) { + req.log.error(`error in POST ${req.originalUrl}`, result.message); res.status(404).send({ error: result.message });- if (result instanceof Error) { - req.log.error(`error in GET /attributes/${req.params.attributeId}`, result.message); + if (result instanceof Error) { + req.log.error(`error in GET ${req.originalUrl}`, result.message); res.status(404).send({ error: result.message }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/controllers/records_controller.ts` around lines 178 - 191, The error logs in the controller hardcode `/attributes/${req.params.attributeId}` causing incorrect paths when the same handlers are reached via `/records`; update the logging in the POST handler and the static async get method to derive the path from the request (e.g., use req.originalUrl or `${req.baseUrl}${req.path}`) instead of the hardcoded string so req.log.error includes the actual invoked route and the error message (refer to the existing req.log.error calls inside the POST response branch and inside static async get).
🧹 Nitpick comments (5)
src/records/record_query.ts (1)
322-328: Filter is fragile to class renames — prefergetDataTypePrefix().The dataType filter still hard-codes the legacy class names (
'KeyValueAttribute','LongTextAttribute','BlobAttribute') and the legacy ID prefix. Any future rename of the wire identifier will silently make this branch dead code (the filter becomes a no-op rather than failing). Consider deriving both the wire identifier and prefix from the record classes themselves so they cannot drift:♻️ Suggested refactor
- if (dataTypeFilter === 'KeyValueAttribute') { - matchedIds = matchedIds.filter((id) => id.startsWith('kv-')); - } else if (dataTypeFilter === 'LongTextAttribute') { - matchedIds = matchedIds.filter((id) => id.startsWith('l-')); - } else if (dataTypeFilter === 'BlobAttribute') { - matchedIds = matchedIds.filter((id) => id.startsWith('bl-')); - } + if (dataTypeFilter) { + const cls = [LongTextRecord, KeyValueRecord, BlobRecord] + .find((c) => c.name === dataTypeFilter + || `${c.name.replace(/Record$/, 'Attribute')}` === dataTypeFilter); + if (cls) { + const prefix = `${cls.getDataTypePrefix()}-`; + matchedIds = matchedIds.filter((id) => id.startsWith(prefix)); + } + }This also future-proofs the dataType filter against the planned rename of the wire identifier itself.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/records/record_query.ts` around lines 322 - 328, Replace the fragile hard-coded class-name branches in the dataTypeFilter block with dynamic lookups using each record class's getDataTypePrefix (or equivalent static method) so the prefix and wire identifier come from the record classes themselves; specifically, remove the literal checks for 'KeyValueAttribute', 'LongTextAttribute', and 'BlobAttribute' and instead map the incoming dataTypeFilter to the corresponding record class (or call a registry/lookup) to obtain its getDataTypePrefix() and then use matchedIds.filter(id => id.startsWith(prefix)); update the logic around the matchedIds filtering (the branch containing matchedIds = matchedIds.filter(...)) and any code that treats dataTypeFilter as a raw string to use the class-derived prefix to avoid brittle renames.src/server/routes.ts (2)
135-146: Optional: factor out the duplicated route registrations.Each handler is now wired twice, once under
/attributes/...and once under/records/.... While intentional for backward compatibility, this is six near-identical pairs that will be easy to drift on (e.g., adding a new route only on one side). Consider a small helper to mount both prefixes from a single declaration:♻️ Suggested refactor
- app.get('/attributes', errorHandler((req, res) => withAuth(req, res, attributesController.index))); - app.post('/attributes', errorHandler((req, res) => withAuth(req, res, attributesController.create))); - app.post('/attribute-compositions', errorHandler((req, res) => withAuth(req, res, attributesController.createComposition))); - app.get('/attributes/:attributeId', errorHandler((req, res) => withAuth(req, res, attributesController.get))); - app.get('/attributes/:attributeId/members', errorHandler((req, res) => withAuth(req, res, attributesController.getMembers))); - app.patch('/attributes/:attributeId', errorHandler((req, res) => withAuth(req, res, attributesController.update))); - app.get('/records', errorHandler((req, res) => withAuth(req, res, attributesController.index))); - app.post('/records', errorHandler((req, res) => withAuth(req, res, attributesController.create))); - app.post('/record-compositions', errorHandler((req, res) => withAuth(req, res, attributesController.createComposition))); - app.get('/records/:attributeId', errorHandler((req, res) => withAuth(req, res, attributesController.get))); - app.get('/records/:attributeId/members', errorHandler((req, res) => withAuth(req, res, attributesController.getMembers))); - app.patch('/records/:attributeId', errorHandler((req, res) => withAuth(req, res, attributesController.update))); + const mountBoth = (method: 'get' | 'post' | 'patch', suffix: string, action) => { + ['/attributes', '/records'].forEach((base) => { + app[method](`${base}${suffix}`, errorHandler((req, res) => withAuth(req, res, action))); + }); + }; + const mountBothCompositions = (action) => { + ['/attribute-compositions', '/record-compositions'].forEach((base) => { + app.post(base, errorHandler((req, res) => withAuth(req, res, action))); + }); + }; + mountBoth('get', '', attributesController.index); + mountBoth('post', '', attributesController.create); + mountBothCompositions(attributesController.createComposition); + mountBoth('get', '/:attributeId', attributesController.get); + mountBoth('get', '/:attributeId/members', attributesController.getMembers); + mountBoth('patch', '/:attributeId', attributesController.update);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/routes.ts` around lines 135 - 146, Routes for the same handlers are duplicated under "/attributes" and "/records" which risks drift; replace the repeated app.<method> registrations by creating a small helper (e.g., mountDualRoutes or mountForPrefixes) that accepts prefixes (['attributes','records']), HTTP method, path suffix (like '/:attributeId/members'), and the handler (withAuth wrapped by errorHandler or pass raw handler and wrap inside), and use it to register routes for attributesController.index, create, createComposition, get, getMembers, update so each handler is registered once for both prefixes; update all duplicated lines that call app.get/post/patch with withAuth/errorHandler wrappers to use this helper and ensure the exact handler symbols (attributesController.index, attributesController.create, attributesController.createComposition, attributesController.get, attributesController.getMembers, attributesController.update) are used.
144-146: Cosmetic::attributeIdparam name retained on/records/...routes.The new record routes still use
:attributeIdas the path parameter name, which the records controller will read asreq.params.attributeId. Functionally fine, but for the long-term migration target it would read more naturally as:recordId. Safe to defer until the records controller is renamed too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/routes.ts` around lines 144 - 146, Rename the route path parameter from :attributeId to :recordId in the three routes that currently mount attributesController handlers so the param reads naturally for records; update the three route definitions that call errorHandler((req, res) => withAuth(req, res, attributesController.get)), errorHandler((req, res) => withAuth(req, res, attributesController.getMembers)), and errorHandler((req, res) => withAuth(req, res, attributesController.update)) to use /records/:recordId, leaving the handler calls (withAuth, errorHandler, and attributesController.*) unchanged so req.params.recordId will be available for future controller renaming.src/records/blob/server/index.ts (1)
7-14: Optional: align default-import bindings with the renamed modules.The default-import paths now resolve under
abstract/abstract_record_serverandabstract/is_record_storage, but the local bindings (AbstractAttributeServer,IsAttributeStorage) and the field type on Line 20 still use the old "Attribute" names. This works (default imports can be named anything), but it leaves a mismatch that future readers may find confusing.♻️ Suggested rename of local bindings
-import AbstractAttributeServer, { LoadResult } from '../../abstract/abstract_record_server'; +import AbstractRecordServer, { LoadResult } from '../../abstract/abstract_record_server'; import SerializedChangeWithMetadata from '../../abstract/serialized_change_with_metadata'; import BlobChange from '../blob_change'; import IsLogger from '../../../../lib/is_logger'; import PsqlStorage from '../../record_storage/psql'; import S3Storage from '../../record_storage/s3'; import PgPoolWithLog from '../../../../lib/pg-log'; -import IsAttributeStorage from '../../abstract/is_record_storage'; +import IsRecordStorage from '../../abstract/is_record_storage'; -export default class BlobRecord extends AbstractAttributeServer< +export default class BlobRecord extends AbstractRecordServer< Blob, BlobChange > { - storage: IsAttributeStorage; + storage: IsRecordStorage;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/records/blob/server/index.ts` around lines 7 - 14, The imports and local bindings use "Attribute" names that no longer match the renamed modules; rename the local default-import bindings AbstractAttributeServer -> AbstractRecordServer and IsAttributeStorage -> IsRecordStorage (and update any references, e.g. the storage field type currently using IsAttributeStorage) so the local identifiers match the module intent (update usages of AbstractAttributeServer and IsAttributeStorage in this file to the new names).src/browser_sdk/index.ts (1)
61-64: Optional: consider a different public name thanRecordfor the new field.Naming the instance member
Recorddoesn't clash with the TypeScript built-inRecord<K, V>utility (different namespaces, and it's used as a type at Lines 34 and 70 of this file without ambiguity), but it's still a footgun for readers. It also makes inline destructuring likeconst { Record } = linkedRecords;shadow the global type within that scope. Given the PR keepsclient.Attributeas a back-compat getter, you could expose this under a less overloaded name (e.g.,Records,Record$, or simply keepAttributeas the primary name) without losing readability.Not blocking — just worth weighing while the public API is being reshaped.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/browser_sdk/index.ts` around lines 61 - 64, The public instance member named Record can be confusing with the TypeScript built-in Record type; rename the field to a clearer name (for example Records or Attributes) while preserving the back-compat getter Attribute (keep get Attribute(): RecordsRepository { return this.NEW_NAME; }) and update all internal references (constructor assignments and any uses of Record, e.g., in linkedRecords destructuring) to the new identifier (NEW_NAME) so the API remains compatible but avoids shadowing the global Record type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@specs.wdio/helpers/session.ts`:
- Around line 5-8: The type alias AttributesRepository is placed between import
statements and breaks the import/first lint rule; move the declaration "type
AttributesRepository = RecordsRepository" so it appears after all import
statements (for example, after the FactsRepository import) to restore proper
import ordering and satisfy the linter.
In `@src/records/key_value/server/index.ts`:
- Line 15: The class was renamed to KeyValueRecord which changes its runtime
.name and can break identity checks; restore the original runtime name by
declaring the class as KeyValueAttribute (i.e., class KeyValueAttribute extends
AbstractAttributeServer...) and then export it under both identifiers so callers
can import either name—keep the default export and the backward-compat re-export
(adjust the existing re-export around KeyValueRecord/KeyValueAttribute) so the
runtime class name remains 'KeyValueAttribute' while preserving the
KeyValueRecord import binding.
---
Outside diff comments:
In `@src/server/controllers/records_controller.ts`:
- Around line 178-191: The error logs in the controller hardcode
`/attributes/${req.params.attributeId}` causing incorrect paths when the same
handlers are reached via `/records`; update the logging in the POST handler and
the static async get method to derive the path from the request (e.g., use
req.originalUrl or `${req.baseUrl}${req.path}`) instead of the hardcoded string
so req.log.error includes the actual invoked route and the error message (refer
to the existing req.log.error calls inside the POST response branch and inside
static async get).
---
Nitpick comments:
In `@src/browser_sdk/index.ts`:
- Around line 61-64: The public instance member named Record can be confusing
with the TypeScript built-in Record type; rename the field to a clearer name
(for example Records or Attributes) while preserving the back-compat getter
Attribute (keep get Attribute(): RecordsRepository { return this.NEW_NAME; })
and update all internal references (constructor assignments and any uses of
Record, e.g., in linkedRecords destructuring) to the new identifier (NEW_NAME)
so the API remains compatible but avoids shadowing the global Record type.
In `@src/records/blob/server/index.ts`:
- Around line 7-14: The imports and local bindings use "Attribute" names that no
longer match the renamed modules; rename the local default-import bindings
AbstractAttributeServer -> AbstractRecordServer and IsAttributeStorage ->
IsRecordStorage (and update any references, e.g. the storage field type
currently using IsAttributeStorage) so the local identifiers match the module
intent (update usages of AbstractAttributeServer and IsAttributeStorage in this
file to the new names).
In `@src/records/record_query.ts`:
- Around line 322-328: Replace the fragile hard-coded class-name branches in the
dataTypeFilter block with dynamic lookups using each record class's
getDataTypePrefix (or equivalent static method) so the prefix and wire
identifier come from the record classes themselves; specifically, remove the
literal checks for 'KeyValueAttribute', 'LongTextAttribute', and 'BlobAttribute'
and instead map the incoming dataTypeFilter to the corresponding record class
(or call a registry/lookup) to obtain its getDataTypePrefix() and then use
matchedIds.filter(id => id.startsWith(prefix)); update the logic around the
matchedIds filtering (the branch containing matchedIds = matchedIds.filter(...))
and any code that treats dataTypeFilter as a raw string to use the class-derived
prefix to avoid brittle renames.
In `@src/server/routes.ts`:
- Around line 135-146: Routes for the same handlers are duplicated under
"/attributes" and "/records" which risks drift; replace the repeated
app.<method> registrations by creating a small helper (e.g., mountDualRoutes or
mountForPrefixes) that accepts prefixes (['attributes','records']), HTTP method,
path suffix (like '/:attributeId/members'), and the handler (withAuth wrapped by
errorHandler or pass raw handler and wrap inside), and use it to register routes
for attributesController.index, create, createComposition, get, getMembers,
update so each handler is registered once for both prefixes; update all
duplicated lines that call app.get/post/patch with withAuth/errorHandler
wrappers to use this helper and ensure the exact handler symbols
(attributesController.index, attributesController.create,
attributesController.createComposition, attributesController.get,
attributesController.getMembers, attributesController.update) are used.
- Around line 144-146: Rename the route path parameter from :attributeId to
:recordId in the three routes that currently mount attributesController handlers
so the param reads naturally for records; update the three route definitions
that call errorHandler((req, res) => withAuth(req, res,
attributesController.get)), errorHandler((req, res) => withAuth(req, res,
attributesController.getMembers)), and errorHandler((req, res) => withAuth(req,
res, attributesController.update)) to use /records/:recordId, leaving the
handler calls (withAuth, errorHandler, and attributesController.*) unchanged so
req.params.recordId will be available for future controller renaming.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91aa8a74-1693-4ba3-9039-8b8ac3dbc8a5
📒 Files selected for processing (35)
specs.wdio/helpers/session.tsspecs.wdio/tinytodo/tinytodo.tsspecs/browser_sdk/attribute.spec.tsspecs/browser_sdk/long_text_attribute.spec.tssrc/browser_sdk/index.tssrc/browser_sdk/records_repository.tssrc/facts/server/auth_cache.tssrc/facts/server/index.tssrc/facts/server/query_subscription_service.tssrc/records/abstract/abstract_record_client.tssrc/records/abstract/abstract_record_server.tssrc/records/abstract/is_record_storage.tssrc/records/abstract/is_serializable.tssrc/records/abstract/serialized_change_with_metadata.tssrc/records/blob/blob_change.tssrc/records/blob/client/index.tssrc/records/blob/server/index.tssrc/records/errors/authorization_error.tssrc/records/key_value/client/index.tssrc/records/key_value/key_value_change.tssrc/records/key_value/server/index.tssrc/records/long_text/client/buffer.tssrc/records/long_text/client/index.tssrc/records/long_text/long_text_change.tssrc/records/long_text/server/index.tssrc/records/record_query.tssrc/records/record_storage/psql/index.tssrc/records/record_storage/psql_with_history/index.tssrc/records/record_storage/s3/index.tssrc/records/record_storage/types.tssrc/server/controllers/records_controller.tssrc/server/middleware/record.tssrc/server/quota/index.tssrc/server/routes.tssrc/server/service_bus_mount.ts
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/content/docs/api-reference.mdx (1)
322-333:⚠️ Potential issue | 🔴 CriticalThe documented
/quota/:nodeIdresponse structure is incorrect and does not match the actual API implementation.The documented example shows
storageUsed,storageLimit, andrecordCountsfields, but the actualQuota.toJSON()method returns:nodeId,userIsAccountable,isUpgraded,totalStorageAvailable,usedStorage,remainingStorageAvailable, andaccounteeInformation.The doc needs a complete rewrite to match the actual response structure at
src/server/quota/index.ts:156-164.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/api-reference.mdx` around lines 322 - 333, The example response for the `/quota/:nodeId` endpoint is wrong—replace the current JSON example with the actual structure returned by Quota.toJSON(): include the fields nodeId, userIsAccountable, isUpgraded, totalStorageAvailable, usedStorage, remainingStorageAvailable, and accounteeInformation (with its subfields) so the docs mirror the implementation in Quota.toJSON(); update the example JSON under the `/quota/:nodeId` section to exactly reflect those property names and types.
🧹 Nitpick comments (4)
docs/content/docs/getting-started.mdx (1)
198-202: Optional: rename leftovertodoAttrtotodoRecordfor consistency.Same applies at Lines 373 and 459. Wire-protocol strings (
KeyValueAttribute) stay, but local variable names in code samples now read inconsistently against the newlr.Record.*API.♻️ Proposed tweak
- const todoAttr = await lr.Record.find(id); - const todoObj = await todoAttr?.getValue(); - - todoAttr?.set({ ...todoObj, completed: checked }); + const todoRecord = await lr.Record.find(id); + const todoObj = await todoRecord?.getValue(); + + todoRecord?.set({ ...todoObj, completed: checked });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/getting-started.mdx` around lines 198 - 202, Rename the leftover variable todoAttr to todoRecord for consistency with the lr.Record.* API: update occurrences where you call lr.Record.find, await todoAttr?.getValue(), and todoAttr?.set(...) to use todoRecord instead (same change needed at the other two examples that reference todoAttr around the other code snippets). Ensure all local variable names in the samples reflect the record terminology while leaving wire-protocol strings (KeyValueAttribute) unchanged.docs/content/docs/authorization-model.mdx (1)
39-152: Optional: renameattributeIdplaceholders torecordIdin example fact triples.Lines 39, 122, 124, 132, 134, 142, 152 all use
attributeIdas the placeholder name even though the surrounding prose now talks exclusively about records. Purely cosmetic, but consistent placeholders make the docs easier to scan.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/authorization-model.mdx` around lines 39 - 152, Update the example fact triples and placeholder variable names that currently use attributeId to use recordId instead (e.g., the triples like [userId, '$isAccountableFor', attributeId], [teamId, '$canRead', attributeId], [teamId, '$canRefine', attributeId], [teamId, '$canReferTo', attributeId], etc.); locate and replace each occurrence of the placeholder attributeId in the examples and surrounding code blocks with recordId so the examples match the surrounding prose about records and remain consistent across examples.docs/content/docs/long-text-attributes.mdx (1)
143-173: Optional: finish the rename inside the editor example.The component prop is
recordIdand state holdsrecord, but the inner async function is stillsetupAttributeand the local handle isattr. Reads inconsistently for a sample developers will copy verbatim.♻️ Proposed tweak
useEffect(() => { - let attr; + let rec; - async function setupAttribute() { - attr = await lr.Record.find(recordId); - if (attr) { + async function setupRecord() { + rec = await lr.Record.find(recordId); + if (rec) { // Subscribe to receive real-time changes from other users - await attr.subscribe(async () => { - setContent(await attr.getValue()); + await rec.subscribe(async () => { + setContent(await rec.getValue()); }); - setRecord(attr); - setContent(await attr.getValue()); + setRecord(rec); + setContent(await rec.getValue()); } } - setupAttribute(); + setupRecord(); return () => { - attr?.unload(); + rec?.unload(); }; }, [recordId, lr]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/long-text-attributes.mdx` around lines 143 - 173, The example mixes names (setupAttribute/attr vs component prop/state record) which is confusing; rename the inner function and local variable to match the component terminology (e.g., change setupAttribute -> setupRecord and attr -> foundRecord), ensure you assign foundRecord = await lr.Record.find(recordId), use foundRecord for subscribe/getValue/unload, call setRecord(foundRecord) and setContent(await foundRecord.getValue()), and leave handleChange using the state record so there’s no shadowing or inconsistent reads.docs/content/docs/query-language.mdx (1)
275-306: Optional: renameattributeIdplaceholders inFact.findAllexamples torecordId.Lines 277, 281, and 305 still read
attributeIdwhile the rest of the page (and PR) standardizes on records. The wire-format identifiers around$hasDataType(Lines 161/169/176/181) correctly remainKeyValueAttribute/LongTextAttribute/BlobAttributeper the backwards-compat strategy — only the placeholder variable names need updating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/query-language.mdx` around lines 275 - 306, Update the placeholder variable name used in the example calls to lr.Fact.findAll — replace attributeId with recordId in the examples that query by subject and object (instances of lr.Fact.findAll shown near the "Find all facts about a specific record" and "Find all facts pointing to a specific record" snippets and the predicate examples where attributeId appears) so the examples match the repo-wide convention; leave the wire-format identifiers like KeyValueAttribute/LongTextAttribute/BlobAttribute unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/content/docs/blob-attributes.mdx`:
- Around line 1-4: The docs pages were retitled but their filenames and the
navigation config still use the old "attributes" slugs; rename the three files
(blob-attributes.mdx, key-value-attributes.mdx, long-text-attributes.mdx) to
match their new titles (e.g., blob-records.mdx, key-value-records.mdx,
long-text-records.mdx) and update the slugs in docs/content/docs/meta.json to
the new names, or alternatively add redirect mappings from the old slugs to the
new slugs in your docs framework’s redirect config; ensure any imports/links
referencing Blob Records, Key-Value Records, or Long Text Records use the
updated filenames/slugs (search for "blob-attributes", "key-value-attributes",
"long-text-attributes" to locate usages).
In `@docs/content/docs/getting-started.mdx`:
- Line 156: The docs import uses missing React exports—useLinkedRecords and
useKeyValueRecords (and related components like LinkedRecordsProvider) do not
exist as `@linkedrecords/react`—so update the repository by either adding a new
publishable package `@linkedrecords/react` that implements and exports
useLinkedRecords, useKeyValueRecords, LinkedRecordsProvider (and wire it into
root package.json exports and docs/package.json), or change the
getting-started.mdx examples to import from existing packages/paths or mark the
React examples as "coming soon"; ensure the symbols useLinkedRecords,
useKeyValueRecords, and LinkedRecordsProvider are implemented and exported if
you choose to create the package, and update package.json exports and docs
dependencies accordingly.
In `@docs/content/docs/index.mdx`:
- Line 19: The docs reference a non-existent React hook useKeyValueRecords and
import it from `@linkedrecords/react`, which will break examples; update index.mdx
by removing or replacing all uses of useKeyValueRecords and any imports from
`@linkedrecords/react` (e.g., the occurrences at line ~19 and others) with either
(a) the correct SDK API calls that actually exist in the package, or (b) a note
and example showing a separate, explicit React integration package if one
exists; ensure all code samples import only real exported symbols from the SDK
and adjust examples to use supported non-React APIs if no React hooks are
provided.
In `@docs/content/docs/quotas.mdx`:
- Around line 182-183: The docs currently mismatch the env var semantics:
QUOTA_COUNT_KV_ATTRIBUTES and QUOTA_COUNT_LT_ATTRIBUTES (kept for
backwards-compatibility and named with the ATTRIBUTES suffix) are boolean
toggles in code (checked via === 'true' in src/server/quota/index.ts), but
quotas.mdx says "records" and configuration.mdx shows numeric "max" examples.
Update quotas.mdx and configuration.mdx to consistently describe these as
boolean feature flags (e.g., "Enable counting of KeyValue attributes in quota —
boolean: set to 'true' to enable") preserve the exact env var names (ATTRIBUTES
suffix), remove numeric examples and "Max key-value records" wording, and add a
short note referencing the code behavior (checked as string 'true') so operators
aren’t confused.
---
Outside diff comments:
In `@docs/content/docs/api-reference.mdx`:
- Around line 322-333: The example response for the `/quota/:nodeId` endpoint is
wrong—replace the current JSON example with the actual structure returned by
Quota.toJSON(): include the fields nodeId, userIsAccountable, isUpgraded,
totalStorageAvailable, usedStorage, remainingStorageAvailable, and
accounteeInformation (with its subfields) so the docs mirror the implementation
in Quota.toJSON(); update the example JSON under the `/quota/:nodeId` section to
exactly reflect those property names and types.
---
Nitpick comments:
In `@docs/content/docs/authorization-model.mdx`:
- Around line 39-152: Update the example fact triples and placeholder variable
names that currently use attributeId to use recordId instead (e.g., the triples
like [userId, '$isAccountableFor', attributeId], [teamId, '$canRead',
attributeId], [teamId, '$canRefine', attributeId], [teamId, '$canReferTo',
attributeId], etc.); locate and replace each occurrence of the placeholder
attributeId in the examples and surrounding code blocks with recordId so the
examples match the surrounding prose about records and remain consistent across
examples.
In `@docs/content/docs/getting-started.mdx`:
- Around line 198-202: Rename the leftover variable todoAttr to todoRecord for
consistency with the lr.Record.* API: update occurrences where you call
lr.Record.find, await todoAttr?.getValue(), and todoAttr?.set(...) to use
todoRecord instead (same change needed at the other two examples that reference
todoAttr around the other code snippets). Ensure all local variable names in the
samples reflect the record terminology while leaving wire-protocol strings
(KeyValueAttribute) unchanged.
In `@docs/content/docs/long-text-attributes.mdx`:
- Around line 143-173: The example mixes names (setupAttribute/attr vs component
prop/state record) which is confusing; rename the inner function and local
variable to match the component terminology (e.g., change setupAttribute ->
setupRecord and attr -> foundRecord), ensure you assign foundRecord = await
lr.Record.find(recordId), use foundRecord for subscribe/getValue/unload, call
setRecord(foundRecord) and setContent(await foundRecord.getValue()), and leave
handleChange using the state record so there’s no shadowing or inconsistent
reads.
In `@docs/content/docs/query-language.mdx`:
- Around line 275-306: Update the placeholder variable name used in the example
calls to lr.Fact.findAll — replace attributeId with recordId in the examples
that query by subject and object (instances of lr.Fact.findAll shown near the
"Find all facts about a specific record" and "Find all facts pointing to a
specific record" snippets and the predicate examples where attributeId appears)
so the examples match the repo-wide convention; leave the wire-format
identifiers like KeyValueAttribute/LongTextAttribute/BlobAttribute unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60461444-0ef7-4ca8-8613-ebae41bf25c6
📒 Files selected for processing (20)
docs/content/docs/accountability.mdxdocs/content/docs/api-reference.mdxdocs/content/docs/authorization-model.mdxdocs/content/docs/blob-attributes.mdxdocs/content/docs/blueprint-pattern.mdxdocs/content/docs/configuration.mdxdocs/content/docs/facts-and-triples.mdxdocs/content/docs/getting-started.mdxdocs/content/docs/index.mdxdocs/content/docs/key-value-attributes.mdxdocs/content/docs/long-text-attributes.mdxdocs/content/docs/meta.jsondocs/content/docs/query-language.mdxdocs/content/docs/query-patterns.mdxdocs/content/docs/quotas.mdxdocs/content/docs/sharing-patterns.mdxdocs/content/docs/team-patterns.mdxdocs/content/docs/terms.mdxlib/client-server-bus/drivers/ws/server/index.tsspecs.wdio/helpers/session.ts
✅ Files skipped from review due to trivial changes (5)
- docs/content/docs/configuration.mdx
- lib/client-server-bus/drivers/ws/server/index.ts
- docs/content/docs/meta.json
- docs/content/docs/facts-and-triples.mdx
- docs/content/docs/blueprint-pattern.mdx
Comment on lines
1
to
4
| --- | ||
| title: Blob Attributes | ||
| title: Blob Records | ||
| description: Binary file storage with S3/MinIO integration | ||
| --- |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find any internal links to the old slugs
fd -e mdx -e md | xargs rg -nP '\(/(blob|key-value|long-text)-attributes' 2>/dev/null
echo "----"
# Find any sidebar/meta config files referencing the slugs
fd -e json -e yaml -e yml -e ts -e tsx -e js | xargs rg -nP '(blob|key-value|long-text)-attributes' 2>/dev/nullRepository: wolfoo2931/linkedrecords
Length of output: 245
🏁 Script executed:
# Check which attribute doc files actually exist
fd -type f -name '*attribute*.mdx' docs/
echo "---"
# View the full meta.json to understand the structure
cat docs/content/docs/meta.jsonRepository: wolfoo2931/linkedrecords
Length of output: 758
🏁 Script executed:
# List all .mdx files in docs/content/docs to see actual filenames
ls -la docs/content/docs/*.mdx | grep -i attribute
echo "---"
# Check the title in each attribute doc file
echo "=== key-value-attributes.mdx ==="
head -5 docs/content/docs/key-value-attributes.mdx
echo ""
echo "=== long-text-attributes.mdx ==="
head -5 docs/content/docs/long-text-attributes.mdx
echo ""
echo "=== blob-attributes.mdx ==="
head -5 docs/content/docs/blob-attributes.mdxRepository: wolfoo2931/linkedrecords
Length of output: 721
Filename and navigation slugs still use old "attributes" terminology across all attribute docs.
All three attribute documentation files have been retitled but not renamed:
key-value-attributes.mdx→ title "Key-Value Records"long-text-attributes.mdx→ title "Long Text Records"blob-attributes.mdx→ title "Blob Records"
The docs/content/docs/meta.json navigation config still references the old slugs (key-value-attributes, long-text-attributes, blob-attributes), creating a mismatch between URL and page title. Rename the files and update meta.json to align with the new naming, or add redirect mappings if your docs framework supports them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/docs/blob-attributes.mdx` around lines 1 - 4, The docs pages
were retitled but their filenames and the navigation config still use the old
"attributes" slugs; rename the three files (blob-attributes.mdx,
key-value-attributes.mdx, long-text-attributes.mdx) to match their new titles
(e.g., blob-records.mdx, key-value-records.mdx, long-text-records.mdx) and
update the slugs in docs/content/docs/meta.json to the new names, or
alternatively add redirect mappings from the old slugs to the new slugs in your
docs framework’s redirect config; ensure any imports/links referencing Blob
Records, Key-Value Records, or Long Text Records use the updated filenames/slugs
(search for "blob-attributes", "key-value-attributes", "long-text-attributes" to
locate usages).
| ```tsx | ||
| // Query data with facts - updates automatically when anything changes | ||
| const todos = useKeyValueAttributes([['$it', 'isA', 'Todo']]); | ||
| const todos = useKeyValueRecords([['$it', 'isA', 'Todo']]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any export/declaration of useKeyValueRecords vs useKeyValueAttributes
rg -nP '\b(useKeyValue(Records|Attributes)|useLongText(Records|Attributes)|useBlob(Records|Attributes))\b' -C2
echo "----"
# Specifically look at hook export sites
fd -t f -e ts -e tsx | xargs rg -nP '^export\s+(const|function)\s+useKeyValue' 2>/dev/nullRepository: wolfoo2931/linkedrecords
Length of output: 4382
🏁 Script executed:
# Search for useKeyValueRecords definition/export in source code (not docs)
rg -t ts -t tsx 'export.*useKeyValueRecords|function useKeyValueRecords|const useKeyValueRecords' src/Repository: wolfoo2931/linkedrecords
Length of output: 96
🏁 Script executed:
# Look at the react hooks or SDK index to see what's actually exported
fd -t f -e ts -e tsx 'hook|index' src/ | head -20Repository: wolfoo2931/linkedrecords
Length of output: 602
🏁 Script executed:
# Check what hooks are exported from the main react entry point
rg -A 5 'export.*from.*hook|export.*useKeyValue' src/ -t ts -t tsx | head -50Repository: wolfoo2931/linkedrecords
Length of output: 96
🏁 Script executed:
# View the browser SDK index to see what hooks are exported
cat -n src/browser_sdk/index.tsRepository: wolfoo2931/linkedrecords
Length of output: 16185
🏁 Script executed:
# Search for useKeyValue in TypeScript files (ts only)
rg -t ts 'export.*useKeyValue|function useKeyValue|const useKeyValue' src/Repository: wolfoo2931/linkedrecords
Length of output: 50
🏁 Script executed:
# Look for any react-specific hooks files
fd -t f -e ts 'react|hook' src/ | grep -v node_modulesRepository: wolfoo2931/linkedrecords
Length of output: 50
🏁 Script executed:
# Find package.json files to identify React package
fd -t f package.json | xargs grep -l reactRepository: wolfoo2931/linkedrecords
Length of output: 86
🏁 Script executed:
# Look for React hooks files/exports
fd -t f -e ts react.*hook src/Repository: wolfoo2931/linkedrecords
Length of output: 50
🏁 Script executed:
# Search for useKeyValue hook definitions anywhere in src/
rg -t ts 'function useKeyValue|export.*useKeyValue|const useKeyValue = ' src/ -A 3Repository: wolfoo2931/linkedrecords
Length of output: 50
🏁 Script executed:
# Look at root package.json
cat package.json | head -50Repository: wolfoo2931/linkedrecords
Length of output: 1524
🏁 Script executed:
# Check if there's a src structure with exports/hooks
ls -la src/ | head -20Repository: wolfoo2931/linkedrecords
Length of output: 466
🏁 Script executed:
# Search for any file that might contain hook definitions (broader search)
rg -t ts 'useKeyValue|useLinkedRecords' src/ --files-with-matchesRepository: wolfoo2931/linkedrecords
Length of output: 50
🏁 Script executed:
# Check the exact imports in the docs
rg -t md 'import.*useKeyValueRecords|import.*useLinkedRecords' docs/Repository: wolfoo2931/linkedrecords
Length of output: 50
🏁 Script executed:
# Check if there's any react-related config or separate react package definition
find . -maxdepth 3 -name "package.json" -type f -exec grep -l "react" {} \;Repository: wolfoo2931/linkedrecords
Length of output: 293
🏁 Script executed:
# Check docs package.json to see what react package it expects
cat -n docs/package.jsonRepository: wolfoo2931/linkedrecords
Length of output: 970
🏁 Script executed:
# Look for any mdx file that imports from `@linkedrecords/react`
rg '@linkedrecords/react' docs/ --type-list | head -10Repository: wolfoo2931/linkedrecords
Length of output: 458
🏁 Script executed:
# Check the exact lines in docs files with the imports
rg -A 2 -B 2 '@linkedrecords/react' docs/Repository: wolfoo2931/linkedrecords
Length of output: 1420
🏁 Script executed:
# Final verification: confirm `@linkedrecords/react` is not a dependency anywhere
grep -r "@linkedrecords/react" . --include="package.json" 2>/dev/null || echo "NOT FOUND in package.json files"Repository: wolfoo2931/linkedrecords
Length of output: 100
🏁 Script executed:
# Check if there's any React hook export in the actual SDK exports
rg -t ts 'export.*react|react.*hook' src/browser_sdk/Repository: wolfoo2931/linkedrecords
Length of output: 50
The useKeyValueRecords hook does not exist in the SDK.
The docs reference this hook at multiple locations (lines 19, 207, 225, 237, 339, 353 in index.mdx) and attempt to import it from @linkedrecords/react. However, this package is not a dependency and no React hooks are exported from the SDK. All import statements will fail at runtime, making every code example in the documentation non-functional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/docs/index.mdx` at line 19, The docs reference a non-existent
React hook useKeyValueRecords and import it from `@linkedrecords/react`, which
will break examples; update index.mdx by removing or replacing all uses of
useKeyValueRecords and any imports from `@linkedrecords/react` (e.g., the
occurrences at line ~19 and others) with either (a) the correct SDK API calls
that actually exist in the package, or (b) a note and example showing a
separate, explicit React integration package if one exists; ensure all code
samples import only real exported symbols from the SDK and adjust examples to
use supported non-React APIs if no React hooks are provided.
The files were renamed in the previous commit but the class names inside were missed. Updates all references across src/, specs/, and lib/. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ORDS Server accepts both old and new names for backwards compatibility. Updates CI, docker-compose, scripts, and docs to use new names. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Flip type definitions: RecordValue/RecordQuery/etc. are now canonical, Attribute* kept as backwards-compat aliases - Rename IsAttributeStorage → IsRecordStorage and all storage methods - Rename isAttributeClientClass, resolveToAttributes, loadAttributes, getAttributeClientId, attributeCache, idToAttribute internals - Update test helpers: canReadTheAttribute → canReadRecord etc. - Update all client.Attribute. → client.Record. in test files - Fix remaining prose in docs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/records/record_query.ts (1)
28-43:⚠️ Potential issue | 🟡 MinorConfirm
isRecordClientClass's by-name fallback still matches every legitimate record client subclass after the rename.The check on Line 36 accepts a class if any prototype on the chain
=== AbstractRecordClientOR has.name === AbstractRecordClient.name(i.e., the literal string"AbstractRecordClient"). After this PR, all in-tree subclasses extend the renamedAbstractRecordClient, so the identity check should suffice. But:
- If any consumer of
record_query.tsships its own bundled copy ofAbstractRecordClient(different module instance), only the by-name fallback saves them — and that fallback now requires the new class name. Anyone still bundling against an older snapshot where the abstract class was namedAbstractAttributeClientwould silently fall through and producefalse, which would then make valid queries get rejected byisValidFactQueryTuple/isValidRecordQuery.- That is unlikely but matches the migration risk: the PR's stated goal is preserving wire/back-compat, while this rename quietly breaks runtime-identity-by-name compatibility for bundled consumers.
If preserving full back-compat for embedders is in scope, consider also matching
proto.name === 'AbstractAttributeClient'here. Otherwise a short comment explaining the intentional break would help.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/records/record_query.ts` around lines 28 - 43, Update isRecordClientClass to preserve runtime compatibility by accepting the legacy class name: in the prototype-walking loop inside isRecordClientClass check both proto === AbstractRecordClient OR proto.name === AbstractRecordClient.name OR proto.name === 'AbstractAttributeClient' so consumers bundling an older-named AbstractAttributeClient still pass the check; update associated references (isValidFactQueryTuple / isValidRecordQuery) behavior remains unchanged and add a brief inline comment mentioning this legacy-name fallback.
🧹 Nitpick comments (9)
Readme.md (1)
78-79: Consider adding a backward-compatibility note for renamed quota env vars.Runtime still accepts
QUOTA_COUNT_KV_ATTRIBUTES/QUOTA_COUNT_LT_ATTRIBUTES(via OR checks insrc/server/quota/index.ts), so a short migration note here would help operators upgrading existing deployments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Readme.md` around lines 78 - 79, Add a short backward-compatibility note to the README explaining that the env vars QUOTA_COUNT_KV_RECORDS and QUOTA_COUNT_LT_RECORDS were renamed from QUOTA_COUNT_KV_ATTRIBUTES and QUOTA_COUNT_LT_ATTRIBUTES, and that the runtime still accepts the old names (see the OR checks in src/server/quota/index.ts) so operators can safely migrate; mention which names are preferred going forward and that both are supported for now.docker-compose.yml (1)
41-42: LGTM — env var rename aligned with the broader migration.One small heads-up: this compose file pins
ghcr.io/wolfoo2931/linkedrecords:latest(line 23). Anyone running with a stale cached image that predates this PR will not recognizeQUOTA_COUNT_KV_RECORDS/QUOTA_COUNT_LT_RECORDS, so quota counting would silently be disabled until theydocker compose pull. Worth a brief mention in release notes / CHANGELOG so existing dev environments know to refresh.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 41 - 42, The docker-compose currently pins the service to ghcr.io/wolfoo2931/linkedrecords:latest which can leave users running a stale image that doesn't recognize the new env vars QUOTA_COUNT_KV_RECORDS / QUOTA_COUNT_LT_RECORDS; update the compose to either use an immutable image reference (digest) or add pull_policy: always / imagePullPolicy equivalent so deploys always pull the latest image, and add a short note to the release notes/CHANGELOG instructing local devs to run docker compose pull (or restart with pull) so quota counting env vars are picked up; reference the image string ghcr.io/wolfoo2931/linkedrecords:latest and the env var names QUOTA_COUNT_KV_RECORDS and QUOTA_COUNT_LT_RECORDS when making the changes.docs/content/docs/quotas.mdx (2)
182-183: Consider documenting backwards compatibility for environment variable names.The environment variable names have been updated from
QUOTA_COUNT_KV_ATTRIBUTES/QUOTA_COUNT_LT_ATTRIBUTEStoQUOTA_COUNT_KV_RECORDS/QUOTA_COUNT_LT_RECORDS. According to the PR commit message, the server accepts both old and new names for backwards compatibility, but this isn't mentioned in the documentation.📝 Suggested addition for clarity
Consider adding a note after the table to clarify backwards compatibility:
| `QUOTA_COUNT_KV_RECORDS` | Count KeyValue records in quota | false | | `QUOTA_COUNT_LT_RECORDS` | Count LongText records in quota | false | +<Callout type="info"> +The old environment variable names `QUOTA_COUNT_KV_ATTRIBUTES` and +`QUOTA_COUNT_LT_ATTRIBUTES` are still supported for backwards compatibility. +</Callout>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/quotas.mdx` around lines 182 - 183, Add a short backwards-compatibility note after the environment variables table explaining that the server accepts both the new variable names `QUOTA_COUNT_KV_RECORDS` / `QUOTA_COUNT_LT_RECORDS` and the previous names `QUOTA_COUNT_KV_ATTRIBUTES` / `QUOTA_COUNT_LT_ATTRIBUTES`; update the docs content near the table entries for `QUOTA_COUNT_KV_RECORDS` and `QUOTA_COUNT_LT_RECORDS` to mention this dual support and recommend using the new names going forward.
182-183: Clarify that these are boolean toggle flags to match code semantics.The descriptions "Count KeyValue records in quota" are ambiguous—these read like configuration options rather than explicit boolean toggles. The code checks
=== 'true'(verified insrc/server/quota/index.ts), confirming boolean semantics. Explicitly stating "boolean" or using "Enable/Whether" phrasing will reduce confusion, especially sinceconfiguration.mdxincorrectly describes these same variables as numeric limits ("Max key-value records" with examples like10000).📝 Suggested clarification
-| `QUOTA_COUNT_KV_RECORDS` | Count KeyValue records in quota | false | -| `QUOTA_COUNT_LT_RECORDS` | Count LongText records in quota | false | +| `QUOTA_COUNT_KV_RECORDS` | Enable counting KeyValue records in quota (boolean) | false | +| `QUOTA_COUNT_LT_RECORDS` | Enable counting LongText records in quota (boolean) | false |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/quotas.mdx` around lines 182 - 183, The docs rows for QUOTA_COUNT_KV_RECORDS and QUOTA_COUNT_LT_RECORDS are ambiguous; update their descriptions to state they are boolean toggle flags (e.g., "Boolean — enable counting KeyValue records in quota") to match the runtime check that compares the env var to 'true' in the quota index code; also correct any conflicting wording in configuration documentation that currently presents them as numeric limits.src/records/record_storage/psql/index.ts (1)
30-30: Class name stillAttributeStorageafter the migration.The default-exported class implements
IsRecordStorageand exclusively exposes*Record*methods, but is still namedAttributeStorage. Consider renaming toRecordStorage(with a back-compat re-export for any remaining importers, mirroring theIsAttributeStorage = IsRecordStoragealias) to finish the rename consistently. The same applies to the siblingpsql_with_history/index.ts.♻️ Sketch
-export default class AttributeStorage implements IsRecordStorage { +export default class RecordStorage implements IsRecordStorage { logger: IsLogger; ... } + +// backwards-compat alias +export { RecordStorage as AttributeStorage };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/records/record_storage/psql/index.ts` at line 30, Rename the default-exported class AttributeStorage to RecordStorage and update its declaration to implement IsRecordStorage (keep the methods unchanged), then add a back-compat export so existing importers still work (e.g., export { RecordStorage as default, RecordStorage as AttributeStorage } or an equivalent re-export) and add the type alias IsAttributeStorage = IsRecordStorage; apply the same rename + back-compat exports/alias in the sibling psql_with_history/index.ts so both modules consistently use RecordStorage while preserving compatibility.src/server/controllers/records_controller.ts (1)
226-242: Optional: rename helpers to match the Record terminology.The private helpers
resolveToAttributesResultToFlatArray(Lines 43, 94, 226, 241) still use the old "Attributes" naming despite operating onResolveToRecordsResult. Since these are private, you can rename toresolveToRecordsResultToFlatArraywithout breaking any consumers, keeping the codebase consistent with the migration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/controllers/records_controller.ts` around lines 226 - 242, The helper method resolveToAttributesResultToFlatArray should be renamed to resolveToRecordsResultToFlatArray to match the new Record terminology; update the private static method declaration (currently private static resolveToAttributesResultToFlatArray) and replace every internal reference/call (e.g., Controller.resolveToAttributesResultToFlatArray(...)) with the new name, preserving the signature and behavior (parameters: result: ResolveToRecordsResult | Record<string,{id:string}> and returned any[]). Ensure no external/public API changes are made and run tests to catch any missed references.src/browser_sdk/records_repository.ts (1)
73-77: Optional: rename residualattributeTypesstatic and localattribute/attr/AttributeClassidentifiers for consistency.The class itself is now
RecordsRepository, but several internal symbols still carry the old name:
static attributeTypes(Line 73) — referenced asRecordsRepository.attributeTypeson Lines 89 and 138.AttributeClasslocals on Lines 88, 92, 100, 137, 141, 145.attribute/attrvariables on Lines 123, 124, 131, 132, 145, 150, 152, 192, 194, 198, 204.- Error strings on Lines 142, 167, 179, 195 (
"Attribute Type ... is unknown","error creating attribute composition","Error transforming id to attribute","Attribute ID ... is unknown").Since
attributeTypesis referenced by name (.attributeTypes) it's effectively part of the static surface; if any external caller depends on it, renaming would need a backwards-compat alias mirroring the pattern used for the class export on Line 282. Functionally fine, just a readability nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/browser_sdk/records_repository.ts` around lines 73 - 77, The repository was renamed to RecordsRepository but several internal symbols still use "attribute"/"Attribute" names which hurts readability; update the static attributeTypes to a more consistent name (e.g., RecordsRepository.recordTypes) and rename local symbols (AttributeClass, attribute/attr variables) and associated error strings ("Attribute Type ... is unknown", "error creating attribute composition", "Error transforming id to attribute", "Attribute ID ... is unknown") to use the new record naming, and if you must preserve backward compatibility keep RecordsRepository.attributeTypes as an alias to the new static name; update references in methods that use AttributeClass, attribute/attr (lines around where RecordsRepository.attributeTypes is referenced) to the new identifiers so names are consistent across the class.specs.wdio/helpers/lr_expects.ts (1)
24-24: Optional: tests still hit the legacy/attributes/...HTTP path.The renamed helpers exercise
${sURL}attributes/${aId}(Lines 24, 79, 86). Since the PR introduces the parallel/records/...routes (and the rest of the SDK now defaults to them), it would be worth adding coverage that the new endpoints also enforce 200/403 / read-vs-write semantics — otherwise a regression on the new route group could ship undetected while these tests stay green via the legacy alias.Also applies to: 79-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs.wdio/helpers/lr_expects.ts` at line 24, The tests currently only exercise the legacy attributes endpoint (the helper function update uses `${sURL}attributes/${aId}` and there are other uses at the same helper locations), so add equivalent test calls that target the new records route to ensure the /records/... endpoints enforce the same 200/403 read-vs-write semantics; specifically, duplicate the fetch calls that use `${sURL}attributes/${aId}` (and the other two occurrences) to also call `${sURL}records/${aId}` (preserving query params and request body/headers) and assert the same success/failure behaviors so the new route group is covered.src/records/record_query.ts (1)
432-438: Inconsistent rename:getAttributeClassByAttributeIdstill uses old "Attribute" terminology but returnsRecordClass.This method is called from multiple external locations (
src/server/middleware/record.ts,src/server/controllers/records_controller.ts, andsrc/records/abstract/abstract_record_server.ts), so renaming requires a backwards-compat alias. Consider either:
- Renaming to
getRecordClassByRecordIdand adding a backwards-compat alias (mirroring the pattern used forgroupAttributeIDsByClasson Line 464), or- At minimum, update the JSDoc/parameter name to reflect that it operates on records.
This is the last public symbol in the file still carrying "Attribute" terminology on the record-side surface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/records/record_query.ts` around lines 432 - 438, The exported method getAttributeClassByAttributeId uses outdated "Attribute" naming but returns a RecordClass; rename it to getRecordClassByRecordId (update its JSDoc and parameter name from "id" to something like "recordId") and keep a backwards-compatible alias named getAttributeClassByAttributeId that forwards to the new function (mirror the alias pattern used by groupAttributeIDsByClass). Update any internal references within this file to call the new getRecordClassByRecordId and ensure the alias is exported so external callers in src/server/middleware/record.ts, src/server/controllers/records_controller.ts, and src/records/abstract/abstract_record_server.ts continue to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Readme.md`:
- Around line 78-79: Replace the user-facing wording "accountee quota" in the
README entries for QUOTA_COUNT_KV_RECORDS and QUOTA_COUNT_LT_RECORDS with the
correct phrase (e.g., "account quota" or "tenant quota" as appropriate); update
both descriptions so they read something like "If the storage space for KeyValue
records is deducted from the account quota" and "If the storage space for
LongText records is deducted from the account quota" respectively.
In `@specs.wdio/tinytodo/tinytodo_expects.ts`:
- Around line 9-14: The clearCache helper is resetting the wrong property
(attributeCache) so the RecordsRepository's actual cache remains stale; inside
function clearCache (the client.do callback using lr) assign an empty object to
lr.Record.recordCache instead of lr.Record.attributeCache so tests clear the
real cache used by RecordsRepository.
In `@src/records/abstract/abstract_record_client.ts`:
- Line 93: The client now hardcodes the /records/* endpoints (see the const url
in AbstractRecordClient and usages in create, getDataURL(), load(), and the
change-stream subscription) which will break against older servers that only
expose /attributes/*; modify AbstractRecordClient to detect server capability
and fall back to /attributes/* when /records/* endpoints are unavailable (either
by querying a server version/capability endpoint or attempting a request and
using /attributes/* on 404/405), and update create, getDataURL(), load, and the
subscription logic to use the selected base path; alternatively add a clear
runtime check that throws a descriptive error instructing users to upgrade the
server and document the required server version in the README/CHANGELOG.
---
Outside diff comments:
In `@src/records/record_query.ts`:
- Around line 28-43: Update isRecordClientClass to preserve runtime
compatibility by accepting the legacy class name: in the prototype-walking loop
inside isRecordClientClass check both proto === AbstractRecordClient OR
proto.name === AbstractRecordClient.name OR proto.name ===
'AbstractAttributeClient' so consumers bundling an older-named
AbstractAttributeClient still pass the check; update associated references
(isValidFactQueryTuple / isValidRecordQuery) behavior remains unchanged and add
a brief inline comment mentioning this legacy-name fallback.
---
Nitpick comments:
In `@docker-compose.yml`:
- Around line 41-42: The docker-compose currently pins the service to
ghcr.io/wolfoo2931/linkedrecords:latest which can leave users running a stale
image that doesn't recognize the new env vars QUOTA_COUNT_KV_RECORDS /
QUOTA_COUNT_LT_RECORDS; update the compose to either use an immutable image
reference (digest) or add pull_policy: always / imagePullPolicy equivalent so
deploys always pull the latest image, and add a short note to the release
notes/CHANGELOG instructing local devs to run docker compose pull (or restart
with pull) so quota counting env vars are picked up; reference the image string
ghcr.io/wolfoo2931/linkedrecords:latest and the env var names
QUOTA_COUNT_KV_RECORDS and QUOTA_COUNT_LT_RECORDS when making the changes.
In `@docs/content/docs/quotas.mdx`:
- Around line 182-183: Add a short backwards-compatibility note after the
environment variables table explaining that the server accepts both the new
variable names `QUOTA_COUNT_KV_RECORDS` / `QUOTA_COUNT_LT_RECORDS` and the
previous names `QUOTA_COUNT_KV_ATTRIBUTES` / `QUOTA_COUNT_LT_ATTRIBUTES`; update
the docs content near the table entries for `QUOTA_COUNT_KV_RECORDS` and
`QUOTA_COUNT_LT_RECORDS` to mention this dual support and recommend using the
new names going forward.
- Around line 182-183: The docs rows for QUOTA_COUNT_KV_RECORDS and
QUOTA_COUNT_LT_RECORDS are ambiguous; update their descriptions to state they
are boolean toggle flags (e.g., "Boolean — enable counting KeyValue records in
quota") to match the runtime check that compares the env var to 'true' in the
quota index code; also correct any conflicting wording in configuration
documentation that currently presents them as numeric limits.
In `@Readme.md`:
- Around line 78-79: Add a short backward-compatibility note to the README
explaining that the env vars QUOTA_COUNT_KV_RECORDS and QUOTA_COUNT_LT_RECORDS
were renamed from QUOTA_COUNT_KV_ATTRIBUTES and QUOTA_COUNT_LT_ATTRIBUTES, and
that the runtime still accepts the old names (see the OR checks in
src/server/quota/index.ts) so operators can safely migrate; mention which names
are preferred going forward and that both are supported for now.
In `@specs.wdio/helpers/lr_expects.ts`:
- Line 24: The tests currently only exercise the legacy attributes endpoint (the
helper function update uses `${sURL}attributes/${aId}` and there are other uses
at the same helper locations), so add equivalent test calls that target the new
records route to ensure the /records/... endpoints enforce the same 200/403
read-vs-write semantics; specifically, duplicate the fetch calls that use
`${sURL}attributes/${aId}` (and the other two occurrences) to also call
`${sURL}records/${aId}` (preserving query params and request body/headers) and
assert the same success/failure behaviors so the new route group is covered.
In `@src/browser_sdk/records_repository.ts`:
- Around line 73-77: The repository was renamed to RecordsRepository but several
internal symbols still use "attribute"/"Attribute" names which hurts
readability; update the static attributeTypes to a more consistent name (e.g.,
RecordsRepository.recordTypes) and rename local symbols (AttributeClass,
attribute/attr variables) and associated error strings ("Attribute Type ... is
unknown", "error creating attribute composition", "Error transforming id to
attribute", "Attribute ID ... is unknown") to use the new record naming, and if
you must preserve backward compatibility keep RecordsRepository.attributeTypes
as an alias to the new static name; update references in methods that use
AttributeClass, attribute/attr (lines around where
RecordsRepository.attributeTypes is referenced) to the new identifiers so names
are consistent across the class.
In `@src/records/record_query.ts`:
- Around line 432-438: The exported method getAttributeClassByAttributeId uses
outdated "Attribute" naming but returns a RecordClass; rename it to
getRecordClassByRecordId (update its JSDoc and parameter name from "id" to
something like "recordId") and keep a backwards-compatible alias named
getAttributeClassByAttributeId that forwards to the new function (mirror the
alias pattern used by groupAttributeIDsByClass). Update any internal references
within this file to call the new getRecordClassByRecordId and ensure the alias
is exported so external callers in src/server/middleware/record.ts,
src/server/controllers/records_controller.ts, and
src/records/abstract/abstract_record_server.ts continue to work.
In `@src/records/record_storage/psql/index.ts`:
- Line 30: Rename the default-exported class AttributeStorage to RecordStorage
and update its declaration to implement IsRecordStorage (keep the methods
unchanged), then add a back-compat export so existing importers still work
(e.g., export { RecordStorage as default, RecordStorage as AttributeStorage } or
an equivalent re-export) and add the type alias IsAttributeStorage =
IsRecordStorage; apply the same rename + back-compat exports/alias in the
sibling psql_with_history/index.ts so both modules consistently use
RecordStorage while preserving compatibility.
In `@src/server/controllers/records_controller.ts`:
- Around line 226-242: The helper method resolveToAttributesResultToFlatArray
should be renamed to resolveToRecordsResultToFlatArray to match the new Record
terminology; update the private static method declaration (currently private
static resolveToAttributesResultToFlatArray) and replace every internal
reference/call (e.g., Controller.resolveToAttributesResultToFlatArray(...)) with
the new name, preserving the signature and behavior (parameters: result:
ResolveToRecordsResult | Record<string,{id:string}> and returned any[]). Ensure
no external/public API changes are made and run tests to catch any missed
references.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fdd9449f-64de-4a9b-880d-6e73e97bb149
📒 Files selected for processing (46)
.github/workflows/wdio-tests.yml.gitignoreCLAUDE.mdReadme.mdbin/test.wdio.shdocker-compose.ymldocs/content/docs/authorization-model.mdxdocs/content/docs/configuration.mdxdocs/content/docs/long-text-attributes.mdxdocs/content/docs/quotas.mdxspecs.wdio/browser_sdk/auth.spec.tsspecs.wdio/browser_sdk/query_subscription.spec.tsspecs.wdio/browser_sdk/quota.spec.tsspecs.wdio/helpers/lr_expects.tsspecs.wdio/helpers/session.tsspecs.wdio/load/documents/documents.spec.tsspecs.wdio/quota_upgrade/paddle.spec.tsspecs.wdio/tinytodo/tinytodo.tsspecs.wdio/tinytodo/tinytodo_expects.tsspecs/browser_sdk/attribute.spec.tsspecs/browser_sdk/blob_attribute.spec.tsspecs/browser_sdk/fact.spec.tsspecs/browser_sdk/key_value_attribute.spec.tsspecs/browser_sdk/long_text_attribute.spec.tssrc/browser_sdk/index.tssrc/browser_sdk/records_repository.tssrc/facts/server/auth_cache.tssrc/facts/server/query_subscription_service.tssrc/records/abstract/abstract_record_client.tssrc/records/abstract/abstract_record_server.tssrc/records/abstract/is_record_storage.tssrc/records/blob/client/index.tssrc/records/blob/server/index.tssrc/records/key_value/client/index.tssrc/records/key_value/server/index.tssrc/records/long_text/client/index.tssrc/records/long_text/server/index.tssrc/records/record_query.tssrc/records/record_storage/psql/index.tssrc/records/record_storage/psql_with_history/index.tssrc/records/record_storage/s3/index.tssrc/records/record_storage/types.tssrc/server/controllers/records_controller.tssrc/server/middleware/record.tssrc/server/quota/index.tssrc/server/service_bus_mount.ts
✅ Files skipped from review due to trivial changes (4)
- CLAUDE.md
- .gitignore
- bin/test.wdio.sh
- docs/content/docs/authorization-model.mdx
🚧 Files skipped from review as they are similar to previous changes (15)
- src/records/abstract/is_record_storage.ts
- src/records/abstract/abstract_record_server.ts
- src/records/long_text/client/index.ts
- src/records/blob/client/index.ts
- docs/content/docs/configuration.mdx
- specs.wdio/tinytodo/tinytodo.ts
- specs.wdio/helpers/session.ts
- src/records/long_text/server/index.ts
- src/server/quota/index.ts
- src/records/key_value/server/index.ts
- src/records/blob/server/index.ts
- src/server/service_bus_mount.ts
- specs/browser_sdk/attribute.spec.ts
- docs/content/docs/long-text-attributes.mdx
- src/records/record_storage/s3/index.ts
| } | ||
|
|
||
| const url = `/attributes?dtp=${this.getDataTypePrefix()}&clientId=${this.clientId}`; | ||
| const url = `/records?dtp=${this.getDataTypePrefix()}&clientId=${this.clientId}`; |
There was a problem hiding this comment.
Client now requires server with /records/* endpoints — upgrade ordering matters.
The SDK has been switched from /attributes/* to /records/* exclusively for create, getDataURL(), load(), and the change-stream subscription. Although the server still serves /attributes/* for backwards compatibility, a newer client released in this PR will fail when pointed at any older server that does not yet expose /records/*. Consider one of:
- Documenting the required server version in CHANGELOG/README so users upgrade the server first.
- Or keeping the client on
/attributes/*(which the new server still serves) until the next major to maintain symmetric backwards compatibility with old servers.
Also applies to: 140-140, 235-235, 268-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/records/abstract/abstract_record_client.ts` at line 93, The client now
hardcodes the /records/* endpoints (see the const url in AbstractRecordClient
and usages in create, getDataURL(), load(), and the change-stream subscription)
which will break against older servers that only expose /attributes/*; modify
AbstractRecordClient to detect server capability and fall back to /attributes/*
when /records/* endpoints are unavailable (either by querying a server
version/capability endpoint or attempting a request and using /attributes/* on
404/405), and update create, getDataURL(), load, and the subscription logic to
use the selected base path; alternatively add a clear runtime check that throws
a descriptive error instructing users to upgrade the server and document the
required server version in the README/CHANGELOG.
attributeCache was renamed to recordCache but the browser-side cache-clearing call in the test helper wasn't updated, causing stale cached records to be returned and authorization checks to be skipped. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
1 participant
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.
Renames src/attributes/ → src/records/, all Attribute classes to Record equivalents (KeyValueRecord, LongTextRecord, BlobRecord, RecordsRepository), and adds /records/* HTTP endpoints alongside the existing /attributes/* routes.
Backwards compatibility is preserved: client.Attribute getter, old class name aliases (KeyValueAttribute etc.), old HTTP routes, and wire protocol strings ('KeyValueAttribute' in $hasDataType queries) all remain unchanged.
Summary by CodeRabbit
New Features
Chores
Documentation