Skip to content

Conversation

@DaxServer
Copy link
Owner

No description provided.

@coderabbitai
Copy link

coderabbitai bot commented Oct 17, 2025

📝 Walkthrough

Summary by CodeRabbit

  • Documentation

    • Added comprehensive coding conventions, product overview, technology stack, and project structure documentation for development guidance.
  • Bug Fixes

    • Improved error handling and detection in data processing features and API responses.
    • Enhanced type safety by removing unsafe type assertions.
  • Chores

    • Updated project dependencies and build tools for improved performance and compatibility.
    • Streamlined CI/CD workflows and configuration files.

Walkthrough

Refactors GitHub Actions workflows to use jq for Bun version extraction, introduces comprehensive project documentation, updates error handling in backend plugins, bumps dependencies across the stack, modernizes async patterns in frontend components, and removes legacy prompt files.

Changes

Cohort / File(s) Summary
GitHub Actions Workflows
.github/workflows/test.yml, .github/workflows/typecheck.yml
Replaced shell-based Bun version extraction (grep/awk/head chains) with structured jq query reading engines.bun from package.json.
Project Documentation
.trae/rules/code-conventions.md, .trae/rules/product.md, .trae/rules/structure.md, .trae/rules/tech.md
Added four new documentation files defining coding conventions, product overview, project structure, and technology stack.
ESLint Configuration
backend/eslint.config.ts
Replaced object spread of tseslint config with direct reference to tseslint.configs.recommended.
ESLint Configuration
frontend/eslint.config.ts
Removed globals imports and standard JavaScript parser defaults (js.configs.recommended, ecmaVersion, sourceType, browser globals augmentation).
Root Workspace Dependencies
package.json
Updated Bun from 1.2.23 to 1.3.0 in engines; bumped Elysia, Eden, TypeScript tooling, and ESLint to newer versions.
Backend Dependencies
backend/package.json
Updated @duckdb/node-api, @elysiajs/cors, @elysiajs/openapi; added typescript-eslint dev dependency.
Frontend Dependencies
frontend/package.json
Updated unhead/vue, primevue, vue-router, and dev dependencies including primevue/auto-import-resolver, tailwindcss, eslint-plugin-vue, vue-tsc.
Backend Source & Error Handling
backend/src/index.ts, backend/src/plugins/error-handler.ts
Added showToolbar: 'never' to OpenAPI scalar config; refactored error handler to use status() builder instead of set, changing VALIDATION and 500 error response patterns.
Backend Tests
backend/tests/api/project/import.test.ts
Updated 500 error assertion to use expect.stringContaining(...) instead of exact string matching.
Frontend Composables & Components
frontend/src/features/project-management/composables/useProjectCreationComposable.ts, frontend/src/features/project-management/composables/useProjectListComposable.ts, frontend/src/features/wikibase-schema/components/SchemaSelector.vue, frontend/src/features/data-processing/components/ColumnHeaderMenu.vue
Converted async/await patterns to promise chains in delete confirmations; removed runtime type assertions in favor of optional chaining; simplified error condition checks.
Frontend Composables & Tests
frontend/src/features/wikibase-schema/composables/useSchemaApi.ts, frontend/src/features/wikibase-schema/composables/__tests__/useSchemaApi.test.ts
Removed TypeScript suppression comments; updated test mock from data: null to data: [] for no-data scenario.
Removed Prompt Documentation
prompts/Backend.md, prompts/Frontend.md
Deleted legacy prompt documentation files.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Elysia as Elysia<br/>Error Handler
    participant Response as Response<br/>Builder

    Note over Elysia: onError Handler Refactor

    Client->>Elysia: Request with validation error
    activate Elysia
    Elysia->>Elysia: Extract {code, error, status}
    alt Validation Error (code === 'VALIDATION')
        Elysia->>Response: status(422, [error.valueError])
    else Other Error
        Elysia->>Response: status(500, internalServerErrorWithData(...))
    end
    Elysia->>Client: Return response
    deactivate Elysia
Loading
sequenceDiagram
    participant User
    participant Component as Vue<br/>Component
    participant Store as Pinia<br/>Store

    Note over Component: Delete Confirmation Flow (Before)
    User->>Component: Trigger delete with async accept
    activate Component
    Component->>Store: await deleteProject()
    activate Store
    Store-->>Component: Deleted
    deactivate Store
    Component->>Component: showSuccess()
    deactivate Component

    Note over Component: Delete Confirmation Flow (After)
    User->>Component: Trigger delete with void accept
    activate Component
    Component->>Store: deleteProject() (no await)
    activate Store
    Store-->>Component: Promise (not awaited)
    deactivate Store
    Component->>Component: .then(() => showSuccess())
    deactivate Component
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

The review spans multiple domains—dependency updates (low complexity but broad scope), configuration changes (medium), backend error handler refactor requiring Elysia semantics knowledge (medium), frontend async-to-promise conversions spread across several files (medium), and new documentation (low complexity). While many changes are homogeneous (e.g., dependency bumps), the logic refactors and configuration adjustments require targeted reasoning per file/cohort.

Possibly related PRs

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description Check ❓ Inconclusive No pull request description was provided by the author. The description field is completely absent, making it impossible to assess whether any stated intent relates to the actual changeset. Given that the description is essentially non-existent rather than explicitly off-topic, this falls into an ambiguous category where the relationship between described and actual changes cannot be evaluated. Add a pull request description that explains the objectives of these changes. Given the scope of the PR—including new governance documentation, code refactoring, and dependency updates—a description would help reviewers understand the rationale and ensure alignment with project goals.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "chore: update dependencies" is partially related to the changeset. While the PR does include significant dependency version updates across multiple package.json files and the root workspace configuration, the changeset encompasses much more substantial changes including the addition of comprehensive governance documentation files (.trae/rules/code-conventions.md, product.md, structure.md, tech.md), code refactoring in error handling and configurations, workflow optimizations, and removal of prompt files. The title refers to a real aspect of the changes (dependency updates) but does not capture the primary scope or most important changes from a holistic perspective.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch update-deps

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.

❤️ Share

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

@DaxServer DaxServer marked this pull request as ready for review October 17, 2025 09:17
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
frontend/src/features/wikibase-schema/composables/__tests__/useSchemaApi.test.ts (1)

403-414: Misalignment between test name and mock behavior—test is now checking a different code path.

The mock change from data: null to data: [] alters which code path is tested:

  • With data: null (original): The condition !data?.data at line 124 is true, triggering the error handler that returns []
  • With data: [] (current): The condition !data?.data is false (empty array is truthy), so execution proceeds to line 129 where .map() on the empty array returns []

Both return [] but through different paths. The test title says "when no data is returned," which describes the null scenario, not an empty array response. All other tests in this file consistently use data: null for similar patterns (lines 158, 175, 223, 293, 391, 458, 479, 520, 534).

Either revert the mock to data: null to match the test intent, or rename the test to reflect the new scenario: "should return empty array when backend returns empty array."

♻️ Duplicate comments (2)
frontend/src/features/data-processing/components/ColumnHeaderMenu.vue (2)

47-65: Same inconsistent error checking pattern.

Line 52 has the same issue as line 32 - the error check was simplified to if (error) but line 53 still accesses error.value, creating the same potential runtime error described in the previous comment.


67-85: Same inconsistent error checking pattern.

Line 72 has the same issue as lines 32 and 52 - the error check was simplified to if (error) but line 73 still accesses error.value, creating the same potential runtime error described in the previous comments.

🧹 Nitpick comments (1)
.trae/rules/code-conventions.md (1)

1-182: Comprehensive coding guidelines documentation.

This documentation provides clear and detailed coding standards covering testing, Vue components, auto-imports, and backend practices. The structure and content are excellent for maintaining consistency across the codebase.

The markdownlint tool identified fenced code blocks without language specifiers at lines 5, 17, and 32. Consider adding language identifiers for better syntax highlighting:

  • Line 5: Add typescript or ts to the code fence
  • Line 17: Add typescript or ts to the code fence
  • Line 32: Already has typescript specified (no change needed)

This is a minor formatting improvement and doesn't affect the documentation's quality or accuracy.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4003be8 and e0c0546.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (26)
  • .github/workflows/test.yml (1 hunks)
  • .github/workflows/typecheck.yml (1 hunks)
  • .trae/rules/code-conventions.md (0 hunks)
  • .trae/rules/code-conventions.md (1 hunks)
  • .trae/rules/product.md (0 hunks)
  • .trae/rules/product.md (1 hunks)
  • .trae/rules/structure.md (0 hunks)
  • .trae/rules/structure.md (1 hunks)
  • .trae/rules/tech.md (0 hunks)
  • .trae/rules/tech.md (1 hunks)
  • backend/eslint.config.ts (1 hunks)
  • backend/package.json (2 hunks)
  • backend/src/index.ts (1 hunks)
  • backend/src/plugins/error-handler.ts (1 hunks)
  • backend/tests/api/project/import.test.ts (1 hunks)
  • frontend/eslint.config.ts (0 hunks)
  • frontend/package.json (2 hunks)
  • frontend/src/features/data-processing/components/ColumnHeaderMenu.vue (3 hunks)
  • frontend/src/features/project-management/composables/useProjectCreationComposable.ts (1 hunks)
  • frontend/src/features/project-management/composables/useProjectListComposable.ts (1 hunks)
  • frontend/src/features/wikibase-schema/components/SchemaSelector.vue (1 hunks)
  • frontend/src/features/wikibase-schema/composables/__tests__/useSchemaApi.test.ts (1 hunks)
  • frontend/src/features/wikibase-schema/composables/useSchemaApi.ts (0 hunks)
  • package.json (2 hunks)
  • prompts/Backend.md (0 hunks)
  • prompts/Frontend.md (0 hunks)
💤 Files with no reviewable changes (4)
  • prompts/Backend.md
  • prompts/Frontend.md
  • frontend/src/features/wikibase-schema/composables/useSchemaApi.ts
  • frontend/eslint.config.ts
🧰 Additional context used
🧬 Code graph analysis (1)
backend/src/plugins/error-handler.ts (1)
backend/src/types/error-handler.ts (1)
  • ApiErrorHandler (6-211)
🪛 LanguageTool
.trae/rules/structure.md

[grammar] ~50-~50: There might be a mistake here.
Context: ...tes grouped by domain (/api/project/*) - Schema definitions in schemas.ts files...

(QB_NEW_EN)


[grammar] ~51-~51: There might be a mistake here.
Context: ...Schema definitions in schemas.ts files - Separate files for each major operation ...

(QB_NEW_EN)


[grammar] ~52-~52: There might be a mistake here.
Context: ... Separate files for each major operation - Tests mirror the API structure ### Fron...

(QB_NEW_EN)


[grammar] ~57-~57: There might be a mistake here.
Context: ... Reusable UI components (PrimeVue-based) - Composables: Business logic and state ...

(QB_NEW_EN)


[grammar] ~58-~58: There might be a mistake here.
Context: ...s**: Business logic and state management - Stores: Global state with Pinia - **Pa...

(QB_NEW_EN)


[grammar] ~59-~59: There might be a mistake here.
Context: ...nt - Stores: Global state with Pinia - Pages: Route-level components - **Type...

(QB_NEW_EN)


[grammar] ~60-~60: There might be a mistake here.
Context: ...inia - Pages: Route-level components - Types: Frontend-specific type definiti...

(QB_NEW_EN)


[grammar] ~65-~65: There might be a mistake here.
Context: ...ot**: Workspace and shared configuration - Backend: Elysia-specific TypeScript co...

(QB_NEW_EN)


[grammar] ~66-~66: There might be a mistake here.
Context: ...end**: Elysia-specific TypeScript config - Frontend: Vite and Vue-specific config...

(QB_NEW_EN)


[grammar] ~67-~67: There might be a mistake here.
Context: ...d**: Vite and Vue-specific configuration - Shared: Base TypeScript config with pa...

(QB_NEW_EN)

.trae/rules/tech.md

[grammar] ~5-~5: There might be a mistake here.
Context: ...ry runtime and package manager (>=1.0.0) - Workspaces: Monorepo with frontend a...

(QB_NEW_EN)


[grammar] ~6-~6: There might be a mistake here.
Context: ...o with frontend and backend packages - TypeScript: Strict mode with ES2022 ta...

(QB_NEW_EN)


[grammar] ~11-~11: There might be a mistake here.
Context: ...sia**: Web framework for API development - DuckDB: High-performance analytical da...

(QB_NEW_EN)


[grammar] ~12-~12: There might be a mistake here.
Context: ...igh-performance analytical database via @duckdb/node-api - SQLite: Project persistence (dataforge...

(QB_NEW_EN)


[grammar] ~13-~13: There might be a mistake here.
Context: ...te**: Project persistence (dataforge.db) - Wikibase SDK: Integration with Wikibas...

(QB_NEW_EN)


[grammar] ~14-~14: There might be a mistake here.
Context: ...K**: Integration with Wikibase instances - Elysia Eden: Type-safe client generati...

(QB_NEW_EN)


[grammar] ~19-~19: There might be a mistake here.
Context: ...tack - Vue 3: Composition API with <script setup lang="ts"> - Vite: Build tool and dev server - **Pi...

(QB_NEW_EN)


[grammar] ~20-~20: There might be a mistake here.
Context: ...>` - Vite: Build tool and dev server - Pinia: State management - PrimeVue...

(QB_NEW_EN)


[grammar] ~21-~21: There might be a mistake here.
Context: ...dev server - Pinia: State management - PrimeVue: UI component library with Pr...

(QB_NEW_EN)


[grammar] ~22-~22: There might be a mistake here.
Context: ...**: UI component library with PrimeIcons - Tailwind CSS: Utility-first styling wi...

(QB_NEW_EN)


[grammar] ~23-~23: There might be a mistake here.
Context: ...y-first styling with PrimeUI integration - Vue Router: Client-side routing - **Vu...

(QB_NEW_EN)


[grammar] ~24-~24: There might be a mistake here.
Context: ...on - Vue Router: Client-side routing - VueUse: Composition utilities ## Deve...

(QB_NEW_EN)


[grammar] ~29-~29: There might be a mistake here.
Context: ... linting with TypeScript and Vue support - Prettier: Code formatting - **Auto-imp...

(QB_NEW_EN)


[grammar] ~30-~30: There might be a mistake here.
Context: ... support - Prettier: Code formatting - Auto-imports: Automatic imports for Vu...

(QB_NEW_EN)


[grammar] ~31-~31: There might be a mistake here.
Context: ... Vue composables and PrimeVue components - TypeScript: Strict type checking acros...

(QB_NEW_EN)


[grammar] ~32-~32: There might be a mistake here.
Context: ...*: Strict type checking across the stack - Bun Test: Native testing framework for...

(QB_NEW_EN)


[grammar] ~76-~76: There might be a mistake here.
Context: ...sting**: Uses Bun's built-in test runner - Test Structure: Tests follow the patte...

(QB_NEW_EN)


[grammar] ~77-~77: There might be a mistake here.
Context: ...Tests follow the pattern *.test.ts or *.spec.ts - Location: Frontend tests in `frontend/...

(QB_NEW_EN)


[grammar] ~78-~78: There might be a mistake here.
Context: ...frontend/src/**/__tests__/ directories - Backend Tests: Backend tests in `backe...

(QB_NEW_EN)

.trae/rules/code-conventions.md

[grammar] ~14-~14: There might be a mistake here.
Context: ...ner for all tests - Import Pattern: import { describe, test, expect } from 'bun:test' - File Naming: Tests must use .test.ts...

(QB_NEW_EN)


[grammar] ~128-~128: There might be a mistake here.
Context: ...e components) - [ ] <style scoped> is at bottom (if present in exception cases) ...

(QB_NEW_EN)


[grammar] ~133-~133: There might be a mistake here.
Context: ... can be replaced with Tailwind utilities - [ ] All styling uses Tailwind classes wh...

(QB_NEW_EN)


[grammar] ~134-~134: There might be a mistake here.
Context: ...ing uses Tailwind classes where possible - [ ] <style> section only exists for co...

(QB_NEW_EN)


[grammar] ~139-~139: There might be a mistake here.
Context: ...etup lang="ts">syntax (Vue components) - [ ] Props defined withdefineProps<Prop...

(QB_NEW_EN)


[grammar] ~140-~140: There might be a mistake here.
Context: ...)andwithDefaults()(Vue components) - [ ] Emits defined withdefineEmits<{}>(...

(QB_NEW_EN)


[grammar] ~141-~141: There might be a mistake here.
Context: ...ith defineEmits<{}>() (Vue components) - [ ] Proper TypeScript typing throughout ...

(QB_NEW_EN)


[grammar] ~146-~146: There might be a mistake here.
Context: ...imported Vue APIs or PrimeVue components - [ ] Uses @frontend/ and @backend/ pa...

(QB_NEW_EN)


[grammar] ~147-~147: There might be a mistake here.
Context: ...` path aliases instead of relative paths - [ ] No import conflicts with auto-import...

(QB_NEW_EN)


[grammar] ~180-~180: There might be a mistake here.
Context: ...s that explain the behavior being tested - Group related tests using describe blo...

(QB_NEW_EN)

🪛 markdownlint-cli2 (0.18.1)
.trae/rules/code-conventions.md

5-5: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


17-17: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


32-32: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🔇 Additional comments (11)
backend/tests/api/project/import.test.ts (1)

69-89: LGTM! More resilient test assertion for dependency updates.

The change to expect.stringContaining(...) appropriately handles potential variations in error message formatting from updated dependencies (likely DuckDB). The test still validates the critical aspects: proper 500 status, error structure, and the core error details about malformed JSON.

frontend/src/features/project-management/composables/useProjectCreationComposable.ts (1)

45-50: LGTM! Improved type safety by removing unsafe type assertions.

The refactor from (data as any)?.data?.id to data?.data?.id is a clean improvement. The conditional on line 45 properly guards the nested property access, and line 50's direct use of data.data.id is safe within the validated block.

frontend/src/features/data-processing/components/ColumnHeaderMenu.vue (1)

27-45: ****

The error handling pattern is correct and follows @elysiajs/eden treaty best practices. The .post() method returns { data, error, ... } where error is nullable—it's either undefined (no error) or an error object with a value property containing the server-returned payload. The pattern if (error) { showError(error.value ...) } properly checks if the error object exists before accessing its value, which is the documented and recommended approach for eden/treaty.

Likely an incorrect or invalid review comment.

backend/src/index.ts (1)

24-24: LGTM!

The addition of showToolbar: 'never' appropriately configures the OpenAPI UI visibility preference.

.trae/rules/product.md (1)

1-19: LGTM! Clear and comprehensive product documentation.

This documentation provides a solid overview of DataForge's purpose, features, and architecture. The distinction from OpenRefine and focus on Wikibase integration is well-articulated.

backend/src/plugins/error-handler.ts (1)

13-22: LGTM! Correctly migrated to updated Elysia error handling API.

The changes align with the Elysia framework update (1.4.9 → 1.4.12), replacing the set parameter with the status() builder function. The error handling logic and behavior remain unchanged.

.github/workflows/test.yml (1)

15-17: LGTM! Improved Bun version extraction with jq.

The migration from shell text parsing to jq-based JSON querying is more robust and less fragile. This directly reads the engines.bun field from package.json without relying on text patterns.

package.json (1)

23-53: All verified workspace dependencies are valid and properly published.

The verification confirms that all specified versions are legitimate and currently available:

  • Bun v1.3 (released 2025-10-10)
  • @elysiajs/eden@1.4.4, elysia@1.4.12, and typescript@5.9.3 all exist on npm

The workspace-wide dependency updates are comprehensive and well-aligned with current ecosystem versions.

backend/package.json (1)

18-21: All dependency versions verified as valid and free from known vulnerabilities.

All three updated dependencies exist in the registry and are unaffected by known security advisories:

  • @duckdb/node-api@1.4.1-r.2: Unaffected (advisory was for v1.3.3)
  • @elysiajs/cors@1.4.0: Unaffected (advisory was for versions < 1.3.1)
  • @elysiajs/openapi@1.4.11: No known vulnerabilities
frontend/package.json (1)

17-64: The Vite 7 results reveal a critical compatibility concern. Let me verify the key compatibility issue with @vitejs/plugin-vue and get Vue Router details:

I found a critical compatibility concern. Let me search for the specific peer dependency constraint:

Potential Vite 7 compatibility issue with @vitejs/plugin-vue requires verification.

Vite 7 now requires Node.js 20.19+, 22.12+ and dropped Node.js 18 support as it reached EOL at the end of April 2025. This should be confirmed in your development and CI environments.

However, the critical concern is compatibility with @vitejs/plugin-vue@5.2.1, which supports peer vite@"^5.0.0 || ^6.0.0". While this shows v5.2.1+ supports Vite 6, I could not locate definitive peer dependency declarations for @vitejs/plugin-vue 6.0.1 with Vite 7.1.10 in publicly available sources.

Historical patterns suggest plugin versions typically follow Vite major versions closely, but without explicit confirmation that @vitejs/plugin-vue 6.0.1 declares Vite 7 compatibility, there's a risk of peer dependency resolution failures or silent incompatibilities during build/runtime.

Verify this before merging:

  • Check the actual peer dependency range declared by @vitejs/plugin-vue 6.0.1 on npm or in its package.json
  • Test full build and dev server locally to ensure no peer dependency conflicts
  • Confirm Node.js version in CI matches the 20.19+ / 22.12+ requirement
backend/eslint.config.ts (1)

9-9: ESLint configuration change is correct and follows flat config best practices.

The removal of the spread operator from tseslint.configs.recommended is appropriate. The defineConfig function automatically flattens any arrays or objects passed to it, so you don't need to spread plugin/config arrays yourself. The current implementation passes configs directly to defineConfig(), which correctly handles the flattening. The syntax is valid and linting behavior remains consistent with modern ESLint flat config patterns.

Comment on lines +21 to 25
accept: () => {
void store.deleteProject(project.id).then(() => {
showSuccess(`Project "${project.name}" has been deleted`)
})
},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add error handling for the delete operation.

The promise chain lacks error handling. If store.deleteProject fails, the error will be unhandled and the user won't be notified. The success message will also not be shown, leaving the user without feedback.

Apply this diff to add proper error handling:

       accept: () => {
         void store.deleteProject(project.id).then(() => {
           showSuccess(`Project "${project.name}" has been deleted`)
+        }).catch((error) => {
+          // Error is already handled by the store/API layer via showError
+          console.error('Failed to delete project:', error)
         })
       },

Alternatively, if the store's deleteProject method already handles errors internally (via showError), consider adding a comment to clarify this:

       accept: () => {
+        // Error handling is managed by store.deleteProject internally
         void store.deleteProject(project.id).then(() => {
           showSuccess(`Project "${project.name}" has been deleted`)
         })
       },
📝 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.

Suggested change
accept: () => {
void store.deleteProject(project.id).then(() => {
showSuccess(`Project "${project.name}" has been deleted`)
})
},
accept: () => {
void store.deleteProject(project.id).then(() => {
showSuccess(`Project "${project.name}" has been deleted`)
}).catch((error) => {
// Error is already handled by the store/API layer via showError
console.error('Failed to delete project:', error)
})
},
🤖 Prompt for AI Agents
In
frontend/src/features/project-management/composables/useProjectListComposable.ts
around lines 21 to 25, the accept callback calls store.deleteProject(project.id)
without handling failures so errors are unhandled and users receive no feedback;
update the accept handler to handle errors by adding a .catch that calls
showError with a descriptive message (and optionally logs the error) and keep
the existing showSuccess in the .then branch (or convert to async/await with
try/catch), or if deleteProject already handles errors internally, add a short
comment stating that fact so reviewers know error handling is delegated.

Comment on lines +87 to 94
accept: () => {
void deleteSchema(projectId.value as UUID, schema.id).then(async () => {
showSuccess(`Schema "${schema.name}" deleted successfully`)
// Refresh the schema list
await loadSchemas()
// Refresh the schema list
await loadSchemas()
})
},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add error handling and simplify the async pattern.

This code has two issues:

  1. Missing error handling: If deleteSchema or loadSchemas fails, errors will be unhandled and the user won't receive feedback.
  2. Mixed async patterns: Combining .then() with an async callback and await inside is inconsistent and unnecessarily complex.

Apply this diff to add error handling and simplify:

-    accept: () => {
-      void deleteSchema(projectId.value as UUID, schema.id).then(async () => {
-        showSuccess(`Schema "${schema.name}" deleted successfully`)
-
-        // Refresh the schema list
-        await loadSchemas()
-      })
+    accept: async () => {
+      try {
+        await deleteSchema(projectId.value as UUID, schema.id)
+        showSuccess(`Schema "${schema.name}" deleted successfully`)
+        // Refresh the schema list
+        await loadSchemas()
+      } catch (error) {
+        // Error is already handled by deleteSchema via showError
+        console.error('Failed to delete schema:', error)
+      }
     },

Alternatively, if error handling is managed internally by deleteSchema, use consistent promise chaining with .catch():

     accept: () => {
       void deleteSchema(projectId.value as UUID, schema.id)
         .then(() => {
           showSuccess(`Schema "${schema.name}" deleted successfully`)
-
-        // Refresh the schema list
-        await loadSchemas()
-      })
+          return loadSchemas()
+        })
+        .catch((error) => {
+          // Error handling is managed by deleteSchema/loadSchemas internally
+          console.error('Operation failed:', error)
+        })
     },
📝 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.

Suggested change
accept: () => {
void deleteSchema(projectId.value as UUID, schema.id).then(async () => {
showSuccess(`Schema "${schema.name}" deleted successfully`)
// Refresh the schema list
await loadSchemas()
// Refresh the schema list
await loadSchemas()
})
},
accept: async () => {
try {
await deleteSchema(projectId.value as UUID, schema.id)
showSuccess(`Schema "${schema.name}" deleted successfully`)
// Refresh the schema list
await loadSchemas()
} catch (error) {
// Error is already handled by deleteSchema via showError
console.error('Failed to delete schema:', error)
}
},
🤖 Prompt for AI Agents
In frontend/src/features/wikibase-schema/components/SchemaSelector.vue around
lines 87 to 94, the accept handler mixes .then with async/await and lacks error
handling; replace it with a single async function using try/catch: await
deleteSchema(projectId.value as UUID, schema.id), on success call showSuccess
and await loadSchemas(), and in catch call showError (or the existing
user-facing error helper) with a clear message and the caught error to surface
failures; alternatively, if deleteSchema already handles errors, use consistent
promise chaining with deleteSchema(...).then(() => { showSuccess(...); return
loadSchemas(); }).catch(err => showError(...)).

@DaxServer DaxServer merged commit ba75c3e into main Oct 17, 2025
6 checks passed
@DaxServer DaxServer deleted the update-deps branch October 17, 2025 09:26
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.

2 participants