Skip to content

Yasasmi/ai knowledge assistant clean - #484

Open
yasasmiwaranga wants to merge 10 commits into
mainfrom
yasasmi/ai-knowledge-assistant-clean
Open

Yasasmi/ai knowledge assistant clean#484
yasasmiwaranga wants to merge 10 commits into
mainfrom
yasasmi/ai-knowledge-assistant-clean

Conversation

@yasasmiwaranga

Copy link
Copy Markdown
Collaborator

No description provided.

@LoopyB LoopyB left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks Yas this is really great - I’ve done a fairly thorough review because this is a substantial feature. The overall AI/RAG structure is promising and there are some strong design choices here, particularly keeping model access server-side and separating retrieval, embedding and generation concerns.

I do think there are a few integration issues that need resolving before merge:

  • a clean checkout can currently fail backend startup because the vector index isn't provisioned (local on your device?)
  • some existing backend dependencies/test configuration have regressed (1axios1, 1mongodb-memory-server1, and the Jest ESM configuration removed/changed).
  • the frontend AI request is hard-coded to localhost rather than using the existing configured/authenticated HTTP client
  • the AI endpoint needs backend authentication/role protection and reasonable request/abuse controls
  • the Docker/deployment setup doesn't yet reproducibly provision Ollama, its models and the vector index and there are Node/dependency compatibility issues.

I'm happy to re-review once done. Hope this helps!

@LoopyB LoopyB left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks Yas - my re-review shows the main integration issues are resolved, and the PR is looking good.

I’ve just got two final items I’d like resolved before merge. The first is the backend Node version: the Docker image is still using Node 18, while some of the retained dependencies require Node 20–22, including pdfjs-dist requiring Node 22.13+. Could you please align the Docker image and package.json engine requirement to Node 22.13+ so the development/deployment runtime matches the dependency requirements and CI environment?

The second is the duplicate AIChatWidget mount on the employer dashboard - ProtectedLayout already provides the widget, so the dashboard-level mount should be removed so we don't have two instances being rendered.

These aren’t additional feature requirements but are just the remaining codebase stability/alignment issues I’d want resolved (as is our goal for sprint 1) before merging a change of this size. Once those two are covered, I’m happy for the remaining AI hardening and UX improvements to be follow-up work rather than holding the PR.

@LoopyB LoopyB left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Well done Yas. I completed final verification and the backend is now aligned to Node 22.13 with matching engine requirements, the retained dependencies are compatible, and the duplicate dashboard widget has been removed while preserving the protected-layout instance. Backend lint and formatting pass, and the previously resolved integration, authentication, rate-limiting, dependency and Docker provisioning fixes remain intact.

No remaining blockers from my end so am approving. Nice work addressing the feedback so thoroughly!

@Krisha190235

Copy link
Copy Markdown
Collaborator

I went through all the commits on this one. Heads-up on a mismatch worth resolving before merge:

Commit 1 ("feat: add AI knowledge assistant") adds a full Ollama RAG stack — src/ai/{chunking,embeddings,indexing,retrieval,controllers}, an aiController.chat endpoint, and the ollama dependency (plus package-lock.json changes). But the PR's net diff is only the knowledge-base data (onboarding.txt, Onboarding.pdf, vectors/onboarding.json) plus the Dockerfile Node bump and a .gitignore tweak. So the AI code + deps look like they were added and then removed across later commits, which leaves the committed vectors with no code on main to generate or read them.

A few things before merge:

Where's the src/ai code landing? If it's a separate PR, let's sequence them so the data and its consumer merge together — otherwise vectors/onboarding.json is orphaned. If the code was dropped intentionally, can you confirm on the Files changed tab that only the 5 data/config files remain?
Please squash-merge. Commit 1 pulls a binary PDF, a large package-lock.json, and the AI code into history before removing them — squashing keeps main clean.
Document the embeddings regen. Since vectors/onboarding.json is committed, add a short note on how to rebuild it (buildIndex.js + Ollama nomic-embed-text, and the vector dimensions) so it doesn't drift from onboarding.txt when the doc changes.
Dockerfile node:18 → 22.13 looks good — it matches backend-lint.yml (Node 22). Just note the assistant needs an Ollama service reachable at :11434, which is a deploy dependency worth calling out.

No secrets in the docs 👍 (the .env section lists key names only, no values.)

Otherwise happy to approve once the code/data sequencing is sorted.

@yasasmiwaranga

Copy link
Copy Markdown
Collaborator Author

Thanks for the review Krisha. I rechecked the final diff against origin/main and confirmed that the complete AI/RAG implementation and the knowledge-base/vector data are both present in the current PR. I also added the requested README documentation covering buildIndex.js, Ollama nomic-embed-text, the 768-dimensional embeddings, vector regeneration, and the Ollama :11434 deployment dependency. The changes have been pushed and the working tree is clean. The PR is ready for final review and squash merge.

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.

3 participants