refactor: reusable tag picking #710
Conversation
WalkthroughAdds a new TagPicker Svelte component and replaces prior TagDropdown usages across dataset, docs (library and document), upload dialog, and run pages. Tag state types change from Set to string[], propagation uses a tags_changed event and simplified bindings; local dropdown/current_tag state removed. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant TP as TagPicker
participant P as Page/Container
participant API as Backend
U->>TP: add / remove / select tag
TP-->>P: tags_changed { previous, current }
P->>P: update local tags (array)
alt Persist change
P->>API: save_tags(current)
API-->>P: 200 OK / error
end
rect rgba(230,245,255,0.6)
note right of TP: honors `disabled`, `initial_expanded`\nand handles Escape/Close to collapse
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)app/web_ui/**/*.{ts,svelte}📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
Files:
app/web_ui/**/*.svelte📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
Files:
app/web_ui/**/*.{ts,tsx,svelte}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📓 Common learnings⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
🔇 Additional comments (5)
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 |
|
CI broken due to Python 3.14 - waiting on #708 to be merged |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
app/web_ui/src/lib/ui/tag_picker.svelte (2)
24-28: Tighten equality check to true set-equality (dedupe both sides).Prevents spurious emissions if previous accidentally contains duplicates.
Apply:
- function tags_are_equal(tags1: string[], tags2: string[]): boolean { - return ( - tags1.length === tags2.length && tags1.every((tag) => tags2.includes(tag)) - ) - } + function tags_are_equal(tags1: string[], tags2: string[]): boolean { + const a = [...new Set(tags1)] + const b = [...new Set(tags2)] + return a.length === b.length && a.every((tag) => b.includes(tag)) + }
82-87: Optional a11y: add aria-labels to icon-only buttons.Improves screen reader usability with no behavior change.
Apply:
- <button + <button + aria-label="Remove tag" class="pl-3 font-medium shrink-0" on:click={() => handle_remove_tag(tag)} {disabled}>✕</button > ... - <button + <button + aria-label="Add tag" class="badge bg-gray-200 text-gray-500 p-3 font-medium {disabled ? 'opacity-50' : ''}" on:click={toggle_dropdown} {disabled}>+</button >Also applies to: 91-97
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/web_ui/src/lib/ui/tag_picker.svelte(1 hunks)app/web_ui/src/routes/(app)/dataset/[project_id]/[task_id]/+page.svelte(7 hunks)app/web_ui/src/routes/(app)/docs/library/[project_id]/+page.svelte(8 hunks)app/web_ui/src/routes/(app)/docs/library/[project_id]/[document_id]/+page.svelte(2 hunks)app/web_ui/src/routes/(app)/docs/library/[project_id]/upload_file_dialog.svelte(6 hunks)app/web_ui/src/routes/(app)/run/run.svelte(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/web_ui/**/*.{ts,svelte}
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
app/web_ui/**/*.{ts,svelte}: Write frontend web code in TypeScript; do not add plain .js sources
From the web UI, make backend calls only to our FastAPI servers; do not call external services directly
Use TypeScript types and interfaces to maintain strong typing in the frontend
Files:
app/web_ui/src/routes/(app)/run/run.svelteapp/web_ui/src/routes/(app)/docs/library/[project_id]/[document_id]/+page.svelteapp/web_ui/src/routes/(app)/docs/library/[project_id]/+page.svelteapp/web_ui/src/routes/(app)/dataset/[project_id]/[task_id]/+page.svelteapp/web_ui/src/lib/ui/tag_picker.svelteapp/web_ui/src/routes/(app)/docs/library/[project_id]/upload_file_dialog.svelte
app/web_ui/**/*.svelte
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
app/web_ui/**/*.svelte: Author components for Svelte v4 compatibility (do not use Svelte v5-only APIs)
Prefer Tailwind CSS and DaisyUI utility/classes for styling in Svelte components
Files:
app/web_ui/src/routes/(app)/run/run.svelteapp/web_ui/src/routes/(app)/docs/library/[project_id]/[document_id]/+page.svelteapp/web_ui/src/routes/(app)/docs/library/[project_id]/+page.svelteapp/web_ui/src/routes/(app)/dataset/[project_id]/[task_id]/+page.svelteapp/web_ui/src/lib/ui/tag_picker.svelteapp/web_ui/src/routes/(app)/docs/library/[project_id]/upload_file_dialog.svelte
app/web_ui/**/*.{ts,tsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for frontend code (including <script lang="ts"> in .svelte files)
Files:
app/web_ui/src/routes/(app)/run/run.svelteapp/web_ui/src/routes/(app)/docs/library/[project_id]/[document_id]/+page.svelteapp/web_ui/src/routes/(app)/docs/library/[project_id]/+page.svelteapp/web_ui/src/routes/(app)/dataset/[project_id]/[task_id]/+page.svelteapp/web_ui/src/lib/ui/tag_picker.svelteapp/web_ui/src/routes/(app)/docs/library/[project_id]/upload_file_dialog.svelte
🧠 Learnings (1)
📓 Common learnings
Learnt from: leonardmq
PR: Kiln-AI/Kiln#546
File: app/web_ui/src/routes/(app)/docs/library/[project_id]/upload_file_dialog.svelte:107-120
Timestamp: 2025-09-10T08:32:18.688Z
Learning: leonardmq prefers to work within the constraints of their SDK codegen for API calls, even when typing is awkward (like casting FormData to match expected types), rather than using alternative approaches like native fetch that would cause compiler errors with their generated types.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#413
File: libs/core/kiln_ai/datamodel/chunk.py:138-160
Timestamp: 2025-08-22T11:17:56.862Z
Learning: leonardmq prefers to avoid redundant validation checks when upstream systems already guarantee preconditions are met. He trusts the attachment system to ensure paths are properly formatted and prefers letting the attachment method (resolve_path) handle any edge cases rather than adding defensive precondition checks.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#390
File: libs/core/kiln_ai/adapters/provider_tools.py:494-499
Timestamp: 2025-07-23T08:58:45.769Z
Learning: leonardmq prefers to keep tightly coupled implementation details (like API versions) hardcoded when they are not user-facing and could break other modules if changed. The shared Config class is reserved for user-facing customizable values, not internal implementation details.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#341
File: libs/server/kiln_server/document_api.py:44-51
Timestamp: 2025-06-18T08:22:58.510Z
Learning: leonardmq prefers to defer fixing blocking I/O in async handlers when: the operation is very fast (milliseconds), user-triggered rather than automated, has no concurrency concerns, and would require additional testing to fix properly. He acknowledges such issues as valid but makes pragmatic decisions about timing the fixes.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#487
File: libs/core/kiln_ai/datamodel/rag.py:33-35
Timestamp: 2025-09-04T06:45:44.212Z
Learning: leonardmq requires vector_store_config_id to be a mandatory field in RagConfig (similar to extractor_config_id, chunker_config_id, embedding_config_id) for consistency. He prefers fixing dependent code that breaks due to missing required fields rather than making fields optional to accommodate incomplete data.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#654
File: app/web_ui/src/routes/(app)/docs/rag_configs/[project_id]/create_rag_config/edit_rag_config_form.svelte:141-152
Timestamp: 2025-09-25T06:38:14.854Z
Learning: leonardmq prefers simple onMount initialization patterns over reactive statements when possible, and is cautious about maintaining internal state for idempotency in Svelte components. He values simplicity and safety in component lifecycle management.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#402
File: libs/core/kiln_ai/adapters/embedding/litellm_embedding_adapter.py:0-0
Timestamp: 2025-07-14T03:43:07.283Z
Learning: leonardmq prefers to keep defensive validation checks even when they're technically redundant, viewing them as useful "quick sanity checks" that provide additional safety nets. He values defensive programming over strict DRY (Don't Repeat Yourself) principles when the redundant code serves as a safeguard.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Desktop Apps (macos-latest)
- GitHub Check: Build Desktop Apps (windows-latest)
- GitHub Check: Build Desktop Apps (ubuntu-22.04)
- GitHub Check: Build Desktop Apps (macos-13)
- GitHub Check: Build Desktop Apps (ubuntu-22.04-arm)
🔇 Additional comments (5)
app/web_ui/src/routes/(app)/run/run.svelte (1)
649-660: Integration looks good; save-on-change wired correctly.tags_changed updates run.tags and persists via PATCH. Assuming TagPicker’s undefined-tags guard is applied, this is solid.
If you keep bind:tags, ensure run.tags is always an array (or rely on the TagPicker fix). Otherwise, consider passing tags={run.tags || []}.
app/web_ui/src/routes/(app)/docs/library/[project_id]/[document_id]/+page.svelte (1)
361-371: Clean swap to TagPicker; event handler persists changes.Good use of tags_changed to update document.tags and call save_tags.
document.tags may be undefined from the API; ensure TagPicker’s undefined handling fix is in place or pass tags={document.tags || []}.
app/web_ui/src/routes/(app)/dataset/[project_id]/[task_id]/+page.svelte (1)
393-397: Good migration to array-based add_tags and TagPicker.State, payloads, and dialog wiring look correct. Disabled condition now simple and accurate.
Also applies to: 433-437, 447-465, 708-736
app/web_ui/src/routes/(app)/docs/library/[project_id]/+page.svelte (1)
384-387: Array-based add_tags + TagPicker integration LGTM.Event wiring and action button disablement are correct. RAG re-run trigger post-edit is a nice touch.
Also applies to: 424-428, 430-464, 707-735
app/web_ui/src/routes/(app)/docs/library/[project_id]/upload_file_dialog.svelte (1)
105-105: LGTM! Clean migration from Set to array.The change from
Set<string>tostring[]forselected_tagsis implemented consistently:
- Type declaration updated (line 105)
- Size check changed to length check (line 145)
- Reset logic updated to empty array (lines 271, 281)
- forEach iteration works correctly on arrays
Also applies to: 145-149, 271-271, 281-281
app/web_ui/src/routes/(app)/docs/library/[project_id]/upload_file_dialog.svelte
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📊 Coverage ReportOverall Coverage: 92% Diff: origin/leonard/kil-128-the-add-document-screen-should-let-me-add-tags...HEADNo lines with coverage information in this diff.
|
leonardmq
force-pushed
the
leonard/kil-128-the-add-document-screen-should-let-me-add-tags
branch
from
8019e26 to
f617653
Compare
leonardmq
force-pushed
the
leonard/kil-141-refactor-consolidate-tag-picking-ui
branch
from
bc7a164 to
64459af
Compare
…dd-tags' of github.com:Kiln-AI/Kiln into leonard/kil-141-refactor-consolidate-tag-picking-ui
…dd-tags' of github.com:Kiln-AI/Kiln into leonard/kil-141-refactor-consolidate-tag-picking-ui
…dd-tags' of github.com:Kiln-AI/Kiln into leonard/kil-141-refactor-consolidate-tag-picking-ui
…dd-tags' of github.com:Kiln-AI/Kiln into leonard/kil-141-refactor-consolidate-tag-picking-ui
…dd-tags' of github.com:Kiln-AI/Kiln into leonard/kil-141-refactor-consolidate-tag-picking-ui
…dd-tags' of github.com:Kiln-AI/Kiln into leonard/kil-141-refactor-consolidate-tag-picking-ui
leonardmq
deleted the
leonard/kil-141-refactor-consolidate-tag-picking-ui
branch
What does this PR do?
Forking off #709 to consolidate the tag picking UI into its own component.
There are 4 instances of tag picking:
This PR wraps the
TagDropdownin a higher-levelTagPickercomponent that handles showing the current tags, emitting an event when overall tag list changes, minimization / expanding.Summary by CodeRabbit
New Features
Refactor
Bug Fixes