fix(wikibase-schema-editor): bugs saving qualifiers and references#121
fix(wikibase-schema-editor): bugs saving qualifiers and references#121
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThis update extends the Wikibase schema editing functionality to support "references" in addition to "qualifiers" for statements. It modifies the editor component, schema store, and related composables to handle references, updates test coverage for these features, and simplifies API payloads for schema updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WikibaseSchemaEditor
participant SchemaStore
participant API
User->>WikibaseSchemaEditor: Edit/Add Statement (with qualifiers, references)
WikibaseSchemaEditor->>SchemaStore: updateStatement/addStatement(property, value, rank, qualifiers, references)
SchemaStore-->>WikibaseSchemaEditor: Statement updated in store (dirty state set)
WikibaseSchemaEditor->>API: saveSchema (includes references)
API-->>WikibaseSchemaEditor: Save response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (7)
frontend/src/features/wikibase-schema/components/WikibaseSchemaEditor.vue (2)
53-54: Add default value to avoid undefined spreads
references?: ReferenceSchemaMapping[]is optional but later always spread (...). Initialise in the type the same asqualifiersto remove optionality:references: ReferenceSchemaMapping[]
236-237: Consider DRYing the repeated default objectThe default statement stub appears three times (lines 54-67, 223-237, 321-334). Extract a small factory function to keep them in sync.
frontend/src/features/wikibase-schema/stores/__tests__/schema.store.test.ts (5)
693-747: LGTM: Qualifiers update path is validated end-to-endThis test robustly covers updating property, value, rank, and qualifiers and correctly checks dirty state transitions. Consider also asserting the statement count remains 1 to guard against accidental replacement vs in-place update:
- const updatedStatement = store.statements.find((s) => s.id === statementId) + expect(store.statements).toHaveLength(1) + const updatedStatement = store.statements.find((s) => s.id === statementId)
749-768: Non-existent update: add a guard assertion to ensure no builder callsSolid check for no crash and no dirty state. To further ensure it’s a true no-op, assert the statement builder wasn’t invoked:
store.updateStatement(nonExistentId, mockProperty, mockValue, 'normal') // Should not crash and should not mark as dirty expect(store.statements).toHaveLength(0) expect(store.isDirty).toBe(false) + expect(mockBuildStatement).not.toHaveBeenCalled()
770-814: LGTM: References update path is validated, including dirty stateLooks good. Optional: if updateStatement preserves existing qualifiers when not provided, you could add an assertion to document that behavior explicitly (helps prevent regressions in merge logic).
// Before updating, optionally set qualifiers: // store.updateStatement(statementId, mockProperty, mockValue, 'normal', mockQualifiers) // After updating with references-only, verify qualifiers remain unchanged: // expect(updatedStatement!.qualifiers).toEqual(mockQualifiers)
816-852: Strengthen: Verify the builder receives references when addingSince addStatement delegates to buildStatement, assert that the references are passed through (helps catch wiring regressions).
- // Add statement with references - const statementId = store.addStatement(mockProperty, mockValue, 'normal', [], mockReferences) + // Add statement with references + mockBuildStatement.mockClear() + const statementId = store.addStatement(mockProperty, mockValue, 'normal', [], mockReferences) + // Ensure references were passed to the builder + const lastCall = mockBuildStatement.mock.calls.at(-1) + expect(lastCall).toEqual([mockProperty, mockValue, 'normal', [], mockReferences])
854-904: Nice: Clear references behavior covered. Consider adding symmetric test for clearing qualifiersThis is a thorough test for clearing references with an empty array and dirty-state verification. For parity and to document store semantics, consider adding a test that clears qualifiers in the same way.
Would you like me to draft the test body? Example:
it('should clear qualifiers when updating statement with empty qualifiers array', () => { const mockProperty: PropertyReference = { id: 'P123', label: 'Test Property', dataType: 'string' } const mockValue: ValueMapping = { type: 'column', source: { columnName: 'test_column', dataType: 'VARCHAR' }, dataType: 'string' } const mockQualifiers: PropertyValueMap[] = [{ property: { id: 'P456', label: 'Qualifier', dataType: 'string' }, value: { type: 'constant', source: 'test value', dataType: 'string' }, }] const statementId = store.addStatement(mockProperty, mockValue, 'normal', mockQualifiers) // Verify qualifiers exist let statement = store.statements.find((s) => s.id === statementId)! expect(statement.qualifiers).toHaveLength(1) store.markAsSaved() expect(store.isDirty).toBe(false) // Clear qualifiers store.updateStatement(statementId, mockProperty, mockValue, 'normal', [], statement.references) statement = store.statements.find((s) => s.id === statementId)! expect(statement.qualifiers).toHaveLength(0) expect(store.isDirty).toBe(true) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/src/features/wikibase-schema/components/WikibaseSchemaEditor.vue(7 hunks)frontend/src/features/wikibase-schema/composables/__tests__/useSchemaApi.test.ts(1 hunks)frontend/src/features/wikibase-schema/composables/useSchemaApi.ts(1 hunks)frontend/src/features/wikibase-schema/composables/useSchemaPersistence.ts(1 hunks)frontend/src/features/wikibase-schema/stores/__tests__/schema.store.test.ts(2 hunks)frontend/src/features/wikibase-schema/stores/schema.store.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>orts-node <file>for running TypeScript or JavaScript files
Do not use dotenv; Bun automatically loads .env files
UseBun.serve()for HTTP servers and WebSockets instead ofexpress
Usebun:sqlitefor SQLite instead ofbetter-sqlite3
UseBun.redisfor Redis instead ofioredis
UseBun.sqlfor Postgres instead ofpgorpostgres.js
Use built-inWebSocketinstead ofws
PreferBun.fileovernode:fs's readFile/writeFile
UseBun.$(e.g.,Bun.$ls``) instead of execa for running shell commands
Files:
frontend/src/features/wikibase-schema/composables/useSchemaApi.tsfrontend/src/features/wikibase-schema/composables/useSchemaPersistence.tsfrontend/src/features/wikibase-schema/stores/schema.store.tsfrontend/src/features/wikibase-schema/stores/__tests__/schema.store.test.tsfrontend/src/features/wikibase-schema/composables/__tests__/useSchemaApi.test.ts
**/*.{html,ts,tsx,css}
📄 CodeRabbit Inference Engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuildfor building HTML, TypeScript, or CSS files
Files:
frontend/src/features/wikibase-schema/composables/useSchemaApi.tsfrontend/src/features/wikibase-schema/composables/useSchemaPersistence.tsfrontend/src/features/wikibase-schema/stores/schema.store.tsfrontend/src/features/wikibase-schema/stores/__tests__/schema.store.test.tsfrontend/src/features/wikibase-schema/composables/__tests__/useSchemaApi.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Use
bun testinstead ofjestfor running tests
Files:
frontend/src/features/wikibase-schema/stores/__tests__/schema.store.test.tsfrontend/src/features/wikibase-schema/composables/__tests__/useSchemaApi.test.ts
🧠 Learnings (2)
📚 Learning: 2025-07-20T14:13:24.706Z
Learnt from: CR
PR: DaxServer/dataforge#0
File: .cursor/rules/frontend-setup.rule.md:0-0
Timestamp: 2025-07-20T14:13:24.706Z
Learning: Applies to src/**/*.{vue,ts} : Use useApi composable (Elysia Eden) for all API calls
Applied to files:
frontend/src/features/wikibase-schema/composables/useSchemaApi.ts
📚 Learning: 2025-07-20T14:13:24.706Z
Learnt from: CR
PR: DaxServer/dataforge#0
File: .cursor/rules/frontend-setup.rule.md:0-0
Timestamp: 2025-07-20T14:13:24.706Z
Learning: Applies to src/**/*.vue : Use storeToRefs for state in components
Applied to files:
frontend/src/features/wikibase-schema/components/WikibaseSchemaEditor.vue
🧬 Code Graph Analysis (1)
frontend/src/features/wikibase-schema/stores/schema.store.ts (2)
frontend/auto-imports.d.ts (6)
UUID(368-368)PropertyReference(371-371)ValueMapping(371-371)StatementRank(371-371)PropertyValueMap(371-371)ReferenceSchemaMapping(371-371)backend/src/api/project/project.wikibase.ts (5)
PropertyReference(35-35)ValueMapping(72-72)StatementRank(97-97)PropertyValueMap(80-80)ReferenceSchemaMapping(88-88)
🪛 GitHub Actions: Typecheck code
frontend/src/features/wikibase-schema/components/WikibaseSchemaEditor.vue
[error] 296-296: TypeScript error TS2345: Argument of type 'string' is not assignable to parameter of type '${string}-${string}-${string}-${string}-${string}'.
🔇 Additional comments (2)
frontend/src/features/wikibase-schema/composables/useSchemaApi.ts (1)
97-100: Confirm backend accepts rawschemaDataYou now send the whole object as-is. If the Eden route still expects only
{ name, wikibase, schema }, this will 422. Double-check the contract.frontend/src/features/wikibase-schema/composables/useSchemaPersistence.ts (1)
37-45: Payload key renamed toschemaJust flagging: older API versions used
item. Ensure backend was updated in sync, otherwise updates will fail.
frontend/src/features/wikibase-schema/composables/__tests__/useSchemaApi.test.ts
Show resolved
Hide resolved
| ( | ||
| property: PropertyReference, | ||
| valueMapping: ValueMapping, | ||
| rank = 'normal', | ||
| qualifiers: PropertyValueMap[] = [], | ||
| references: ReferenceSchemaMapping[] = [], | ||
| ) => ({ | ||
| id: TEST_STATEMENT_ID, | ||
| property, | ||
| value: valueMapping, | ||
| rank, | ||
| qualifiers, | ||
| references, | ||
| }), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Missing type imports for PropertyValueMap and ReferenceSchemaMapping (TS compile fix)
Both PropertyValueMap and ReferenceSchemaMapping are referenced in this new mock signature and return value but are not imported in this file, which will cause TypeScript errors.
Proposed import (outside this range; adjust the source module if different in your repo):
// At the top near other type imports
import type {
ColumnMapping,
PropertyReference,
ValueMapping,
PropertyValueMap,
ReferenceSchemaMapping,
} from '@backend/api/project/project.wikibase'If these types live elsewhere, run this to locate their definitions and correct the import:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where these types are exported
rg -n "export (type|interface) ReferenceSchemaMapping" -A 3
rg -n "export (type|interface) PropertyValueMap" -A 3
# Inspect project.wikibase exports for related types
fd -a "project.wikibase" | xargs -r rg -n "export (type|interface) (ReferenceSchemaMapping|PropertyValueMap|PropertyReference|ValueMapping|ColumnMapping)" -A 2Length of output: 2410
Add missing type imports for qualifiers and references
The test mock now references PropertyValueMap and ReferenceSchemaMapping but they aren’t imported, causing TS errors. Please update the top of frontend/src/features/wikibase-schema/stores/tests/schema.store.test.ts to include:
import type {
ColumnMapping,
PropertyReference,
ValueMapping,
PropertyValueMap,
ReferenceSchemaMapping,
} from '@backend/api/project/project.wikibase'• Verify this path matches your tsconfig “@backend/api” alias.
• (Optional) Strengthen coverage by:
– Asserting the builder receives references when adding a statement.
– Adding a symmetric test for clearing qualifiers (like you have for references).
– On non-existent updates, asserting that the mockBuildStatement isn’t called.
🤖 Prompt for AI Agents
In frontend/src/features/wikibase-schema/stores/__tests__/schema.store.test.ts
around lines 17 to 30, the types PropertyValueMap and ReferenceSchemaMapping
used in the test mock are not imported, causing TypeScript errors. Fix this by
adding an import statement at the top of the file importing these types along
with ColumnMapping, PropertyReference, and ValueMapping from
'@backend/api/project/project.wikibase'. Also verify the import path matches the
tsconfig alias. Optionally, improve test coverage by adding assertions for
references handling, clearing qualifiers, and ensuring mockBuildStatement is not
called on non-existent updates.
| const updateStatement = ( | ||
| statementId: UUID, | ||
| property: PropertyReference, | ||
| value: ValueMapping, | ||
| rank: StatementRank, | ||
| qualifiers: PropertyValueMap[] = [], | ||
| references: ReferenceSchemaMapping[] = [], | ||
| ) => { | ||
| const statementIndex = statements.value.findIndex((s) => s.id === statementId) | ||
|
|
||
| if (statementIndex !== -1) { | ||
| const existingStatement = statements.value[statementIndex] | ||
| if (existingStatement) { | ||
| statements.value[statementIndex] = { | ||
| id: existingStatement.id, | ||
| property, | ||
| value, | ||
| rank, | ||
| qualifiers, | ||
| references, | ||
| } | ||
| markDirty() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
updateStatement blindly overwrites & silently no-ops
- The whole object is rebuilt, so any later-added fields (e.g.
sources) will be lost. - Nothing is returned/throwed when
statementIdis missing – callers can’t tell they updated nothing.
Consider:
- if (statementIndex !== -1) {
- const existingStatement = statements.value[statementIndex]
- if (existingStatement) {
- statements.value[statementIndex] = {
- id: existingStatement.id,
- property,
- value,
- rank,
- qualifiers,
- references,
- }
- markDirty()
- }
- }
+ if (statementIndex === -1) {
+ return false
+ }
+ const existing = statements.value[statementIndex]
+ statements.value[statementIndex] = {
+ ...existing, // future-proof
+ property,
+ value,
+ rank,
+ qualifiers,
+ references,
+ }
+ markDirty()
+ return true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const updateStatement = ( | |
| statementId: UUID, | |
| property: PropertyReference, | |
| value: ValueMapping, | |
| rank: StatementRank, | |
| qualifiers: PropertyValueMap[] = [], | |
| references: ReferenceSchemaMapping[] = [], | |
| ) => { | |
| const statementIndex = statements.value.findIndex((s) => s.id === statementId) | |
| if (statementIndex !== -1) { | |
| const existingStatement = statements.value[statementIndex] | |
| if (existingStatement) { | |
| statements.value[statementIndex] = { | |
| id: existingStatement.id, | |
| property, | |
| value, | |
| rank, | |
| qualifiers, | |
| references, | |
| } | |
| markDirty() | |
| } | |
| } | |
| } | |
| const updateStatement = ( | |
| statementId: UUID, | |
| property: PropertyReference, | |
| value: ValueMapping, | |
| rank: StatementRank, | |
| qualifiers: PropertyValueMap[] = [], | |
| references: ReferenceSchemaMapping[] = [], | |
| ) => { | |
| const statementIndex = statements.value.findIndex((s) => s.id === statementId) | |
| if (statementIndex === -1) { | |
| return false | |
| } | |
| const existing = statements.value[statementIndex] | |
| statements.value[statementIndex] = { | |
| ...existing, // future-proof | |
| property, | |
| value, | |
| rank, | |
| qualifiers, | |
| references, | |
| } | |
| markDirty() | |
| return true | |
| } |
🤖 Prompt for AI Agents
In frontend/src/features/wikibase-schema/stores/schema.store.ts around lines 174
to 198, the updateStatement function overwrites the entire statement object,
causing loss of any additional fields like 'sources', and it silently does
nothing if the statementId is not found. To fix this, modify the function to
merge the updated fields into the existing statement object instead of replacing
it entirely, preserving other fields. Also, add a return value or throw an error
when the statementId is not found to inform callers that no update occurred.
1 participant
No description provided.