fix: resolve ambiguous description column in model search

Open LucasInsight opened this issue 1 month ago • 1 comments

Summary

Fixes an issue where the model search query triggered an ambiguous description column reference, causing inconsistent filtering behavior.

What Changed

Updated SQL in SearchModels to explicitly reference models.description
Ensures correct matching across model_name, description, and tags

Why This Matters

Prevents SQL ambiguity errors
Improves search accuracy and result consistency
No impact on other query paths

Testing

Verified keyword search works correctly
Confirmed no regression on vendor filtering
No schema changes required

Additional Notes

No breaking changes
Safe to merge ✅

Summary by CodeRabbit

New Features
- Added configuration file-based synchronization for models and vendors via uploaded models.json files
- Enhanced sync wizard with file upload capability and preview functionality to compare configuration data with local state before synchronization
- Added conflict detection and resolution options for configuration-based syncing

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Nov 26 '25 15:11 LucasInsight

Walkthrough

Adds config-file-based model synchronization alongside upstream sync: backend handlers for preview and apply, extensive refactor of upstream sync flow into a unified syncContext, frontend file-upload UI and hooks to preview and apply config-based sync, and small query and routing updates.

Changes

Cohort / File(s)	Summary
Ignore Configuration `\`.gitignore``	Added local development and env-specific ignore entries: `local-dev.sh`, `Agents.md`, `CLAUDE.md`, `.env.local`, `docker-compose.local.yml`, `Dockerfile.local`, `package-lock.json`.
Backend Synchronization Logic `\`controller/model_sync.go``	Large refactor: introduced `syncContext`, `syncResult`, conflict tracking types and helpers; replaced hard-coded upstream URL logic with locale-aware fetching; added `parseUpstreamJSON`, fetch/parse helpers, preview endpoint (`SyncUpstreamPreview`) and config endpoints (`SyncConfigPreview`, `SyncConfigModels`); added transactional apply logic, vendor resolution, and error/success payload structures.
Database Query `\`model/model_meta.go``	Qualified columns in `SearchModels` WHERE clause to use `models.` table alias for `model_name`, `description`, and `tags`.
API Routing `\`router/api-router.go``	Added routes: `POST /sync_config/preview` → `SyncConfigPreview` and `POST /sync_config` → `SyncConfigModels`.
Frontend Dependencies `\`web/package.json``	Added dependency: `antd` version `^5.29.1`.
Frontend UI Components `\`web/src/components/table/models/ModelsActions.jsx``	Added config-sync flow: new props `previewConfigDiff` and `syncConfig`; state for `syncSource` and `configContext`; logic to preview config diffs, route conflict resolution to config path, and invoke config sync when selected.
Component Integration `\`web/src/components/table/models/index.jsx``	Passed `previewConfigDiff` and `syncConfig` props into `ModelsActions` from `modelsData`.
Sync Wizard UI `\`web/src/components/table/models/modals/SyncWizardModal.jsx``	Added file upload UI for `models.json` when option is `config`; manages `file` state, enables Next only when a file is selected for config, and includes `file` in onConfirm payload.
Frontend API Hooks `\`web/src/hooks/models/useModelsData.jsx``	Added `previewConfigDiff({ file, locale })` and `syncConfig({ file, locale, overwrite = [] })` to build multipart form data, POST to new backend endpoints, handle responses, loading state, and trigger data refresh on success.

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant UI as Frontend UI
    participant Hook as useModelsData Hook
    participant API as Backend API
    participant DB as Database

    rect rgb(240, 248, 255)
    Note over User,API: Config File Preview Flow
    User->>UI: Select "config" option & upload models.json
    UI->>Hook: previewConfigDiff(file, locale)
    Hook->>API: POST /api/models/sync_config/preview (multipart)
    API->>API: Parse uploaded JSON, build upstreamData
    API->>DB: Load local models & vendors
    API->>API: Build syncContext, calculate conflicts & missing items
    API-->>Hook: Return diff (missing, conflicts, source)
    Hook-->>UI: Display diff & conflicts
    end

    rect rgb(240, 255, 240)
    Note over User,DB: Config Sync Apply Flow
    User->>UI: Provide conflict resolutions (optional) and confirm
    UI->>Hook: syncConfig(file, locale, overwrite)
    Hook->>API: POST /api/models/sync_config (multipart + overwrite JSON)
    API->>API: Parse config & overwrite, build syncContext
    API->>DB: Create/update vendors and models transactionally
    API-->>Hook: Return syncResult (counts, lists)
    Hook->>UI: Refresh vendors/models view and show result
    end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Areas needing extra attention:
- controller/model_sync.go: syncContext design, conflict calculation, vendor resolution, error handling paths, and transactional apply logic.
- Frontend integration: ModelsActions.jsx and SyncWizardModal.jsx—file handling, state transitions, and conditional flows between upstream vs. config paths.
- useModelsData.jsx: multipart construction, API contract alignment, and refresh triggering.
- router/api-router.go & model/model_meta.go: ensure routing and query changes align with DB and middleware expectations.

Poem

🐰 I found a file beneath the log,
I nibbled diffs and chased each cog,
Previewed the dance of change and quirks,
Then pushed the sync—no missing perks.
Hooray, models hop in tidy flocks! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The PR title focuses on fixing an ambiguous column reference in model search, but the changeset introduces extensive new features including config-based sync workflow, file uploads, new API routes, and UI components.	Update the title to reflect the primary changes: 'feat: add config-based model synchronization with file upload and preview'. The ambiguous column fix is a minor housekeeping detail, not the main change.
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

🧪 Generate unit tests (beta)

[ ] Create PR with unit tests
[ ] Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 384ad28698c4b59b2190a095e85fc0a9e06ce570 and 6d601bcee9081eeb2b055b6310d20df8a7398d21.

📒 Files selected for processing (1)

.gitignore (1 hunks)

🔇 Additional comments (3)

.gitignore (3)

1-37: Inconsistency between PR title and AI-generated summary.

The PR title states: "fix: resolve ambiguous description column in model search" (focusing on a SQL query fix), but the AI summary describes extensive backend and frontend refactors for config-based model synchronization. Only the .gitignore file has been provided for review; the actual SQL changes and model search fixes are not included.

Please confirm whether all affected files have been provided (expected: controller/model_sync.go, router/api-router.go, frontend components, and the SQL query fix).

26-35: Local development and secrets files are appropriately excluded.

The additions for local scripts (local-dev.sh), LLM agent prompts (Agents.md, CLAUDE.md), and local configuration files (.env.local, docker-compose.local.yml, Dockerfile.local) are correct and follow best practices for keeping sensitive or environment-specific files out of version control.

36-36: This repository uses a monorepo structure with independent package managers per subdirectory. The ./electron/ directory manages npm dependencies with its own package-lock.json, while the ./web/ directory uses bun with bun.lock. A root-level package-lock.json entry is irrelevant to this setup, and ignoring it at the root is appropriate.

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.}

Nov 26 '25 15:11 coderabbitai[bot]