feat: upgrade onboarding UX and hire-scoped chat ingestion - #11
Merged
Merged
Conversation
Improve the manager and dashboard experience while making hire source sync ingest real URL content and return richer, scoped chat citations so the MVP feels polished and answers are better grounded. Made-with: Cursor
|
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 41 minutes and 53 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 (10)
📝 WalkthroughWalkthroughThis pull request introduces hire-scoped knowledge source ingestion, access control enforcement, and UI component standardization. New components (AppButton, SectionCard, StatusBadge) replace inline button/container markup across multiple pages. The chat handler enforces access control via Changes
Sequence Diagram(s)sequenceDiagram
participant Mgr as Manager
participant VEC as Vectorizer
participant URL as URL Ingestion
participant HTTP as HTTP/Google API
participant SB as Supabase
Mgr->>VEC: Sync hire-scoped source
VEC->>URL: fetchUrlDocument(source.url)
URL->>URL: Parse & validate URL
alt Google Docs URL
URL->>HTTP: GET docs.google.com/export?format=txt
HTTP->>URL: Text content
else Other URL
URL->>HTTP: GET URL with User-Agent
HTTP->>URL: HTML response
URL->>URL: Extract title & strip HTML
end
URL->>VEC: {id, title, content, url}
VEC->>VEC: Compute scopeToken & external_id
VEC->>SB: Upsert document with hire scope
SB->>VEC: Confirm
VEC->>Mgr: Sync complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
…nescaping' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
src/components/ui/StatusBadge.tsx (1)
1-4: Consider wideningchildrentoReactNode.Restricting
childrentostringblocks future use cases like inline icons or<span>accents inside the badge. The render path doesn't depend on it being a string.♻️ Proposed change
-import { ReactNode } from "react"; +import { ReactNode } from "react"; + type StatusBadgeProps = { tone: "success" | "warning" | "neutral"; - children: string; + children: ReactNode; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/StatusBadge.tsx` around lines 1 - 4, The StatusBadgeProps type restricts children to string which prevents passing icons or spans; change its children type to React.ReactNode (or import { ReactNode } and use ReactNode) and update the StatusBadge component/any usage signatures to accept that type so the component can render elements as well as strings.src/app/globals.css (1)
18-23: Dark-mode override is now identical to:root— consider removing or restoring a real light palette.
@media (prefers-color-scheme: dark) :rootredeclares--background/--foregroundto the same values already set on:root. If the app is intentionally dark-only, drop this block; otherwise, define distinct light values on:rootso dark mode actually overrides.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/globals.css` around lines 18 - 23, The dark-mode media block (`@media` (prefers-color-scheme: dark) { :root { --background; --foreground; } }) currently duplicates the same values as :root, so either remove this dark-mode block if the app is dark-only, or restore a distinct light palette by moving dark values into the media block and setting different light values on :root (update the --background/--foreground variable declarations accordingly); locate the :root declarations and the `@media` (prefers-color-scheme: dark) { :root { --background; --foreground; } } block and apply one of the two fixes.src/lib/vectorizer.ts (2)
55-75: Consider chunking large fetched documents before embedding.
resolveSourceDocumentaccepts whateverfetchUrlDocumentreturns and hands the full body togenerateEmbedding(doc.content)later. A long Google Doc / web page will exceed the embedding model's input limit, causing the whole source to be dropped (embeddingFailed += 1) without partial coverage. A simple chunker (e.g. ~1–2k chars with overlap, one upsert row per chunk withexternal_id = ${hireId}:${source.id}#${chunkIdx}) would keep retrieval useful for long sources.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/vectorizer.ts` around lines 55 - 75, resolveSourceDocument currently returns the entire fetched document content which can exceed embedding model limits; change resolveSourceDocument to chunk fetched.content into ~1500–2000 char chunks with ~200 char overlap and return a list of SyncDocument-like chunk objects (or otherwise emit multiple documents) instead of a single doc, assigning unique ids using sourceExternalId(source) + `#${chunkIdx}` and preserving title/url/scope/scopeToken per chunk; ensure downstream code that calls generateEmbedding and upsert handles multiple chunks (loop over chunks, call generateEmbedding per chunk and upsert each with the chunked external_id) so large pages are partially embedded rather than dropped.
47-49:inferProvideris just a pass-through toproviderForType.Drop the wrapper and call
providerForTypedirectly at the call site to remove indirection.♻️ Proposed fix
-function inferProvider(type: KnowledgeSourceType): "notion" | "google_drive" | "slack" | "manual" { - return providerForType(type); -} - @@ - provider: inferProvider(source.type), + provider: providerForType(source.type),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/vectorizer.ts` around lines 47 - 49, The function inferProvider is a redundant passthrough to providerForType; remove the inferProvider declaration and update all call sites to invoke providerForType(...) directly (search for inferProvider and replace with providerForType), ensuring imports/exports remain correct and no other references to inferProvider remain in the codebase.src/lib/ingestion/url.ts (1)
11-22: HTML entity decoding is incomplete.The function only decodes 4 named entities (
,&,<,>) and does not handle:
- Numeric entities (
&#NN;,&#xNN;e.g.,',/)- Common named entities (
",',—, etc.)This causes incomplete HTML entity decoding in fetched content, polluting data quality. Consider using a lightweight library like
string-strip-html(actively maintained, ~130KB, handles both named and numeric entities) to handle both tag stripping and entity decoding in one pass. Alternatively, combine existing libraries likehefor entity decoding only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/ingestion/url.ts` around lines 11 - 22, The stripHtmlTags function only decodes a few named entities and misses numeric and many named entities; replace or augment it to perform full HTML entity decoding (including &#NN; and &#xNN; forms and common named entities like ", ', —). Locate the stripHtmlTags function and either (A) use a lightweight library that both strips tags and decodes entities (e.g., string-strip-html) to handle tags + entities in one pass, or (B) keep tag-stripping but run the result through a robust HTML entity decoder (e.g., he.decode) before trimming; ensure the updated implementation still removes script/style blocks and collapses whitespace as before and expose the same function name stripHtmlTags for callers.src/app/manager/page.tsx (1)
109-135: Consider extracting the filter-pill style or adding a dedicated AppButton variant.The hire filter buttons combine
AppButton variant="ghost"with a long customclassNamethat overrides several base utilities (rounded, py, border color, bg, text color). Two consecutive call sites duplicate this exact selection-state logic, and the overrides depend on Tailwind class source order to resolve conflicts (e.g.,rounded-fullover baserounded-md,py-1over basepy-2). This works today but is fragile if AppButton's defaults change.Consider either:
- Adding a
pill/filtervariant toAppButtonto avoid the override-stack pattern, or- Extracting a small
HireFilterButtoncomponent that encapsulates the active/inactive styling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/manager/page.tsx` around lines 109 - 135, The filter pill buttons duplicate complex Tailwind overrides on AppButton (see AppButton usage, selectedHireId, setActiveHireId and people mapping) which is fragile; fix by extracting the styling and state logic into a single reusable component or variant: either add a new AppButton variant (e.g., "pill" or "filter") that encapsulates rounded-full, px-3 py-1 and the active/inactive border/bg/text classes, or create a small HireFilterButton component that accepts props (hireId, name, isActive, onClick) and applies the correct classes internally, then replace both the "All Employees" and people.map call sites to use that new variant/component so the style logic is centralized.src/app/dashboard/page.tsx (1)
315-326: Recommendrel="noopener noreferrer"on external links.
rel="noreferrer"already impliesnoopenerin modern browsers, but spelling both out is the conventional, defensive form fortarget="_blank"links and avoids any ambiguity for older user agents. Sincesource.urloriginates from manager-attached input, it's worth being explicit here.🛡️ Proposed change
- <a - href={source.url} - target="_blank" - rel="noreferrer" - className="text-xs font-semibold text-cyan-200 underline-offset-2 hover:underline" - > + <a + href={source.url} + target="_blank" + rel="noopener noreferrer" + className="text-xs font-semibold text-cyan-200 underline-offset-2 hover:underline" + >Note: React 19 rejects
javascript:URLs inhrefautomatically, so script-injection via the URL itself is already mitigated by the framework.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/dashboard/page.tsx` around lines 315 - 326, Update the external link anchor rendering that uses source.url (the <a> element with target="_blank" showing source.title) to include rel="noopener noreferrer" instead of just "noreferrer"; this keeps target="_blank" links defensive across user agents and avoids potential window.opener issues—locate the anchor that checks source.url and replace the rel value accordingly.src/app/manager/tasks/page.tsx (1)
211-215: Inconsistent button refactor: hire/duplicate pill buttons remain as raw<button>.These pill-style selectors (lines 211–215 hire selection and 275–278 duplicate-target selection) are still raw
<button>elements with hardcoded Tailwind, while every other action control in this file was migrated toAppButton. Same situation insrc/app/manager/page.tsxlines 109–135.For consistency and to avoid the override-stack pattern (custom
rounded-fulloverridingAppButton's baserounded-md), consider either keeping all pill toggles as raw buttons (and not migrating any) or introducing a smallpill/togglevariant inAppButton.Also applies to: 275-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/manager/tasks/page.tsx` around lines 211 - 215, The hire and duplicate-target pill selectors (the map rendering that calls setSelectedHireId and updates setForm, referencing selectedHireId, hires, and the duplicate-target block) are still raw <button> elements while other controls use AppButton; either revert the other AppButton migrations to raw buttons or add a pill/toggle variant to AppButton and use it here to avoid class-override conflicts: extend AppButton (e.g., add a "pill" or "toggle" variant) to apply rounded-full, px-3 py-1, text-xs and conditional selected styles, then replace the raw buttons in the hires map and the duplicate-target selection to call the same onClick handlers (setSelectedHireId, setForm) but via AppButton with the new variant so styling and behavior are consistent with other controls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/qa-flow.mjs`:
- Around line 103-115: The no-Todo branch currently re-reads dashboard progress
into after via getDashboardProgress(page) immediately after before, making the
comparison a no-op; either remove the redundant getDashboardProgress call and
replace it with an informational log (e.g., use logBug at info level or a log
message) stating "no Todo tasks available, skipping progress check" so the QA
run intentionally skips validation, or insert a deliberate action between
readings (for example a short wait and a page.reload or a navigation) so that
calling getDashboardProgress(page) again can detect transient regressions;
update the branch around todoTask.count(), getDashboardProgress(page), and
logBug accordingly to implement one of these two behaviors.
In `@src/app/api/chat/route.ts`:
- Around line 63-67: The excerpt always appends "..." regardless of length;
update the mapping that builds sources (the validDocs.map that creates
ChatSource objects) to only append "..." when d.content.length > 180 (e.g., use
a conditional to take the first 180 chars and add "..." only if longer), and
guard null/undefined d.content by defaulting to an empty string before slicing
so the excerpt logic in route.ts is robust.
- Line 85: The response currently may set sources to undefined which violates
the ChatResponse type (sources: ChatSource[]); update the return in route.ts
(the NextResponse.json call in the chat route) to always return an array (e.g.,
use sources || [] or sources.length ? sources : []) so ChatResponse.sources is
never undefined, or alternatively update the ChatResponse type to make sources
optional (sources?: ChatSource[]) if undefined is intended—prefer returning an
empty array to keep the existing non-optional type.
In `@src/components/ui/AppButton.tsx`:
- Around line 16-23: The AppButton component currently leaves the native button
type unset which defaults to "submit" inside forms; update the AppButton
function to set a default type of "button" (while still allowing callers to
override via props) by ensuring the rendered <button> includes a type attribute
that uses props.type || "button" (locate in the AppButton function and
AppButtonProps usage to adjust the defaulting behavior).
In `@src/lib/ingestion/url.ts`:
- Around line 41-51: fetchGoogleDocText may store HTML signin/permission pages
for non-public Google Docs because the export endpoint can return 200 with HTML;
update fetchGoogleDocText to detect non-plain-text responses and return null so
callers can fallback. After fetching (in fetchGoogleDocText), check
res.headers.get("content-type") and ensure it indicates text/plain (or starts
with "text/") and additionally inspect the body string for obvious HTML markers
(e.g., startsWith "<!DOCTYPE", "<html", or contains "accounts.google.com" /
"Sign in") and if any of these conditions are true return null rather than the
HTML; keep existing behavior of returning the trimmed text when it is valid
plain text. Ensure you still use googleDocIdFromUrl to skip invalid URLs and
preserve USER_AGENT header usage.
- Around line 41-51: Both fetch paths (notably fetchGoogleDocText) lack an
AbortSignal timeout and a response-size guard; update fetchGoogleDocText and the
other generic fetch function to use an AbortController with a short timeout
(e.g., configurable ms) and to read the response body as a stream, counting
bytes and aborting + returning null if the byte count exceeds a configured max
(size cap) before calling .text(); preserve the USER_AGENT header and ensure the
controller is cleaned up on success/failure and that non-ok responses still
return null.
- Around line 53-108: The fetchUrlDocument function currently fetches arbitrary
URLs and must be hardened: restrict allowed schemes to only http/https (validate
new URL(rawUrl).protocol), resolve parsed.hostname to IP(s) (use DNS lookup) and
reject loopback, RFC1918/private, link-local, and other reserved ranges (e.g.
127.0.0.0/8, ::1, 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, 169.254.0.0/16,
fc00::/7, fe80::/10) before making the request, and set fetch options to disable
automatic redirects (redirect: "manual") then validate any Location header on
redirects against the same host/IP allowlist before following; also add a
request timeout (e.g. AbortSignal.timeout(30000)) and a response body size cap
(stop/abort if body grows past a safe limit before calling res.text()), and
improve stripHtmlTags/entity decoding to handle numeric (&#xNN;/&#NN;) and other
named entities so injected content can't leak — apply these changes around
fetchUrlDocument (and the fetch call), fetchGoogleDocText interaction,
USER_AGENT usage, and stripHtmlTags implementation.
In `@src/lib/vectorizer.ts`:
- Line 151: The ternary that sets external_id is redundant (external_id:
doc.scope === "global" ? doc.id : doc.id); simplify the assignment by replacing
the ternary with a single value assignment so external_id is set directly from
doc.id (locate the external_id assignment in the document construction within
src/lib/vectorizer.ts, e.g., where external_id is assigned from doc).
---
Nitpick comments:
In `@src/app/dashboard/page.tsx`:
- Around line 315-326: Update the external link anchor rendering that uses
source.url (the <a> element with target="_blank" showing source.title) to
include rel="noopener noreferrer" instead of just "noreferrer"; this keeps
target="_blank" links defensive across user agents and avoids potential
window.opener issues—locate the anchor that checks source.url and replace the
rel value accordingly.
In `@src/app/globals.css`:
- Around line 18-23: The dark-mode media block (`@media` (prefers-color-scheme:
dark) { :root { --background; --foreground; } }) currently duplicates the same
values as :root, so either remove this dark-mode block if the app is dark-only,
or restore a distinct light palette by moving dark values into the media block
and setting different light values on :root (update the
--background/--foreground variable declarations accordingly); locate the :root
declarations and the `@media` (prefers-color-scheme: dark) { :root { --background;
--foreground; } } block and apply one of the two fixes.
In `@src/app/manager/page.tsx`:
- Around line 109-135: The filter pill buttons duplicate complex Tailwind
overrides on AppButton (see AppButton usage, selectedHireId, setActiveHireId and
people mapping) which is fragile; fix by extracting the styling and state logic
into a single reusable component or variant: either add a new AppButton variant
(e.g., "pill" or "filter") that encapsulates rounded-full, px-3 py-1 and the
active/inactive border/bg/text classes, or create a small HireFilterButton
component that accepts props (hireId, name, isActive, onClick) and applies the
correct classes internally, then replace both the "All Employees" and people.map
call sites to use that new variant/component so the style logic is centralized.
In `@src/app/manager/tasks/page.tsx`:
- Around line 211-215: The hire and duplicate-target pill selectors (the map
rendering that calls setSelectedHireId and updates setForm, referencing
selectedHireId, hires, and the duplicate-target block) are still raw <button>
elements while other controls use AppButton; either revert the other AppButton
migrations to raw buttons or add a pill/toggle variant to AppButton and use it
here to avoid class-override conflicts: extend AppButton (e.g., add a "pill" or
"toggle" variant) to apply rounded-full, px-3 py-1, text-xs and conditional
selected styles, then replace the raw buttons in the hires map and the
duplicate-target selection to call the same onClick handlers (setSelectedHireId,
setForm) but via AppButton with the new variant so styling and behavior are
consistent with other controls.
In `@src/components/ui/StatusBadge.tsx`:
- Around line 1-4: The StatusBadgeProps type restricts children to string which
prevents passing icons or spans; change its children type to React.ReactNode (or
import { ReactNode } and use ReactNode) and update the StatusBadge component/any
usage signatures to accept that type so the component can render elements as
well as strings.
In `@src/lib/ingestion/url.ts`:
- Around line 11-22: The stripHtmlTags function only decodes a few named
entities and misses numeric and many named entities; replace or augment it to
perform full HTML entity decoding (including &#NN; and &#xNN; forms and common
named entities like ", ', —). Locate the stripHtmlTags function
and either (A) use a lightweight library that both strips tags and decodes
entities (e.g., string-strip-html) to handle tags + entities in one pass, or (B)
keep tag-stripping but run the result through a robust HTML entity decoder
(e.g., he.decode) before trimming; ensure the updated implementation still
removes script/style blocks and collapses whitespace as before and expose the
same function name stripHtmlTags for callers.
In `@src/lib/vectorizer.ts`:
- Around line 55-75: resolveSourceDocument currently returns the entire fetched
document content which can exceed embedding model limits; change
resolveSourceDocument to chunk fetched.content into ~1500–2000 char chunks with
~200 char overlap and return a list of SyncDocument-like chunk objects (or
otherwise emit multiple documents) instead of a single doc, assigning unique ids
using sourceExternalId(source) + `#${chunkIdx}` and preserving
title/url/scope/scopeToken per chunk; ensure downstream code that calls
generateEmbedding and upsert handles multiple chunks (loop over chunks, call
generateEmbedding per chunk and upsert each with the chunked external_id) so
large pages are partially embedded rather than dropped.
- Around line 47-49: The function inferProvider is a redundant passthrough to
providerForType; remove the inferProvider declaration and update all call sites
to invoke providerForType(...) directly (search for inferProvider and replace
with providerForType), ensuring imports/exports remain correct and no other
references to inferProvider remain in the codebase.
🪄 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: cb4e6171-f7f4-4e98-b5d8-af90a3bf9c43
📒 Files selected for processing (14)
README.mdscripts/qa-flow.mjssrc/app/api/chat/route.tssrc/app/dashboard/page.tsxsrc/app/globals.csssrc/app/layout.tsxsrc/app/manager/page.tsxsrc/app/manager/tasks/page.tsxsrc/components/ui/AppButton.tsxsrc/components/ui/SectionCard.tsxsrc/components/ui/StatusBadge.tsxsrc/lib/ingestion/url.tssrc/lib/types.tssrc/lib/vectorizer.ts
Address security and review findings by adding SSRF-safe URL fetching, content chunking for embeddings, and UI/button consistency fixes while keeping chat/source typing and QA behavior robust. Made-with: Cursor
Resolve remaining regex and URL host-sanitization findings in the URL ingestion pipeline to satisfy high-severity security checks. Made-with: Cursor
This was referenced Apr 25, 2026
Merged
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
2 participants
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.
Improve the manager and dashboard experience while making hire source sync ingest real URL content and return richer, scoped chat citations so the MVP feels polished and answers are better grounded.
Made-with: Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Style
Documentation