feat(kanban): unify cloud + local boards on the document model#45
Open
cnjack wants to merge 3 commits into
Open
feat(kanban): unify cloud + local boards on the document model#45cnjack wants to merge 3 commits into
cnjack wants to merge 3 commits into
Conversation
Retire the orphaned cloud-DB kanban (③) and converge on the document-backed board (.md cards + .board views synced via the documents table) as the single source of truth, then rebuild every relational feature on documents.id. Cleanup / retire - Remove Kanban.tsx + /kanban route, handlers/kanban/*, kanban_* models, hub kanban events, kanban_tests/e2e; migration 0018 drops all 9 kanban_* tables. - Strip cloud-DB kanban from MCP/CLI. Kanban MCP + CLI (rebuilt on the document model) - Separate kanban MCP server at POST /mcp/kanban (mcp/kanban_tools.rs, ServerKind). - Local-first `jtype board/card` CLI on jtype-core (list_boards, set_frontmatter_field). Feature parity (shared BoardSurface) - Colored labels (resolveTags/autoTagColor), member assignee (listMembers), activity timeline (document_versions + author username). Comments / webhooks / ticket-links (fresh, keyed by documents.id) - 0019 card_comments; 0020 webhooks + delivery worker re-homed into save_document_version (covers REST + sync push + conflict + live), SSRF-hardened (IPv4-mapped/fe80 IPv6 + delivery-time DNS-rebinding re-check); 0021 card_tickets + board_sequences for /browse/OCCSV-3371 (atomic LAST_INSERT_ID allocator, idempotent per document). Verified: jtype-core/jtype-web/jtype-cli cargo check --tests, web tsc+build, desktop tsc. Findings from adversarial review fixed (LAST_INSERT_ID CAST AS SIGNED, idempotent allocate, webhook SSRF + sync-push trigger gap). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR fully retires the cloud DB kanban subsystem: it drops all ChangesKanban Unification v2
Sequence Diagram(s)sequenceDiagram
participant CLI_MCP as jtype-cli (--kanban)
participant MCPKanban as POST /mcp/kanban
participant kanban_tools
participant VaultDocs as Vault .board/.md docs
participant SaveDoc as save_document_version
participant WebhookEnqueue as enqueue_event
participant DeliveryWorker as webhook_delivery worker
participant ExternalWebhook as External webhook target
CLI_MCP->>MCPKanban: JSON-RPC tools/call (create_card)
MCPKanban->>kanban_tools: call("create_card", args)
kanban_tools->>VaultDocs: scan .board files → find board
kanban_tools->>VaultDocs: save new .md card with frontmatter
VaultDocs-->>kanban_tools: saved
kanban_tools-->>MCPKanban: isError: false
Note over SaveDoc,DeliveryWorker: When WebBoardView saves a card
SaveDoc->>VaultDocs: save_document_version_inner
SaveDoc->>WebhookEnqueue: fire_card_webhook (board_ref, card metadata)
WebhookEnqueue->>WebhookEnqueue: INSERT webhook_deliveries (board_ref IS NULL OR matches)
DeliveryWorker->>WebhookEnqueue: poll due deliveries
DeliveryWorker->>DeliveryWorker: ssrf_blocked(target_url)
DeliveryWorker->>ExternalWebhook: HMAC-signed POST payload
ExternalWebhook-->>DeliveryWorker: 200 OK
DeliveryWorker->>WebhookEnqueue: UPDATE webhook_deliveries status=succeeded
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.0)services/jtype-web/src/lib.rsThanks 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/jtype-web/src/handlers/webhooks.rs (1)
107-126: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winBlock all non-global IP ranges in the webhook SSRF guard.
The shared predicate still allows non-global targets such as IPv4 CGNAT
100.64.0.0/10, IPv4 multicast/reserved ranges, and IPv6 multicast/documentation ranges. Since both create-time validation and the delivery-time DNS recheck use this path, those addresses can still be reached by webhook delivery.🛡️ Proposed hardening
pub(crate) fn is_blocked_v4(ip: std::net::Ipv4Addr) -> bool { + let octets = ip.octets(); ip.is_loopback() || ip.is_private() || ip.is_link_local() || ip.is_unspecified() || ip.is_broadcast() - || ip.octets()[0] == 0 + || octets[0] == 0 + || (octets[0] == 100 && (octets[1] & 0b1100_0000) == 0b0100_0000) // 100.64.0.0/10 + || (octets[0] == 198 && (octets[1] == 18 || octets[1] == 19)) // 198.18.0.0/15 + || octets[0] >= 224 // multicast + reserved } pub(crate) fn is_blocked_v6(ip: std::net::Ipv6Addr) -> bool { @@ if let Some(v4) = ip.to_ipv4_mapped().or_else(|| ip.to_ipv4()) { return is_blocked_v4(v4); } + let segments = ip.segments(); ip.is_loopback() || ip.is_unspecified() - || (ip.segments()[0] & 0xfe00) == 0xfc00 // ULA fc00::/7 - || (ip.segments()[0] & 0xffc0) == 0xfe80 // link-local fe80::/10 + || (segments[0] & 0xfe00) == 0xfc00 // ULA fc00::/7 + || (segments[0] & 0xffc0) == 0xfe80 // link-local fe80::/10 + || (segments[0] & 0xff00) == 0xff00 // multicast ff00::/8 + || (segments[0] == 0x2001 && segments[1] == 0x0db8) // documentation 2001:db8::/32 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/jtype-web/src/handlers/webhooks.rs` around lines 107 - 126, The is_blocked_v4 and is_blocked_v6 functions do not block all non-global IP ranges, allowing CGNAT ranges like 100.64.0.0/10, IPv4 multicast ranges, IPv6 multicast ranges, and IPv6 documentation ranges through. Add additional checks to is_blocked_v4 to block IPv4 multicast (224.0.0.0/4) and CGNAT (100.64.0.0/10) ranges by checking the first octet and appropriate bits, and add checks to is_blocked_v6 to block IPv6 multicast (ff00::/8) and documentation ranges (2001:db8::/32) by examining the segments, or alternatively use the is_global() method if available to return true only for genuinely global addresses.services/jtype-web/src/handlers/mod.rs (1)
5-20: 📐 Maintainability & Code Quality | 🟠 MajorAdd integration tests for comments, tickets, and webhooks handlers.
The
comments,tickets, andwebhookshandler modules define multiple API endpoints (list_comments,create_comment,delete_comment,allocate,list_tickets,resolve_ticket,browse,list_webhooks,create_webhook,delete_webhook) that require corresponding test files. Per the coding guidelines, all handlers inservices/jtype-web/src/handlers/**/*.rsmust have integration tests. Createcomments_tests.rs,tickets_tests.rs, andwebhooks_tests.rsunderservices/jtype-web/tests/with coverage for each endpoint using the web-service test harness (common::setup().await,tower::ServiceExt::oneshot,common::uid(),common::wname()).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/jtype-web/src/handlers/mod.rs` around lines 5 - 20, Create three new integration test files in services/jtype-web/tests/ named comments_tests.rs, tickets_tests.rs, and webhooks_tests.rs to test the handler modules exported in mod.rs. Each test file should contain test cases covering all endpoints defined in the respective handler modules: comments_tests.rs should test list_comments, create_comment, and delete_comment endpoints; tickets_tests.rs should test allocate, list_tickets, and resolve_ticket endpoints; webhooks_tests.rs should test browse, list_webhooks, create_webhook, and delete_webhook endpoints. Each test should use common::setup().await to initialize the test harness, tower::ServiceExt::oneshot to send requests to the service, and common::uid() and common::wname() for test data generation as needed.Source: Coding guidelines
🧹 Nitpick comments (1)
shared/components/board/BoardSurface.tsx (1)
720-724: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse semantic shared color tokens for the ticket badge.
The new shared badge adds palette-specific color utilities; use semantic token classes so desktop and web theming stay aligned. As per coding guidelines,
shared/**/*.{ts,tsx,css}: “Shared components must use semantic Tailwind classes only (e.g.text-brand,bg-brand-soft) fromshared/styles/tokens.cssand never hardcode hex values.”Proposed tokenized badge colors
- <span className="mb-0.5 inline-block rounded bg-stone-100 px-1 py-0.5 font-mono text-[10px] font-medium tracking-tight text-stone-500"> + <span className="mb-0.5 inline-block rounded bg-brand-soft px-1 py-0.5 font-mono text-[10px] font-medium tracking-tight text-brand-dark">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shared/components/board/BoardSurface.tsx` around lines 720 - 724, The ticket badge in the span element uses hardcoded Tailwind color classes (bg-stone-100 and text-stone-500) instead of semantic color tokens. Replace these palette-specific utilities with semantic token classes from shared/styles/tokens.css following the shared component guidelines. Identify the span element displaying the card.ticket and swap the hardcoded color classes (bg-stone-100 for background and text-stone-500 for text) with the appropriate semantic token classes (e.g., bg-brand-soft, text-brand patterns) that maintain consistency across desktop and web theming.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal-docs/kanban/ticket-links.md`:
- Around line 40-59: The ticket_key column in the card_tickets table definition
is using VARCHAR(12), but the actual migration uses VARCHAR(16). Update the
card_tickets table definition to change the ticket_key column from VARCHAR(12)
to VARCHAR(16) to align the design document with the implemented schema
migration. The board_sequences table ticket_key column can remain as VARCHAR(12)
since that matches the migration.
- Around line 97-104: The browse handler in handlers/tickets.rs (lines 214-248)
queries ticket key-number pairs across multiple workspaces without a
deterministic ordering in the SQL WHERE clause, causing arbitrary results when
duplicates exist across workspaces. Fix this by either adding an ORDER BY clause
to the query that uses a stable tie-breaker like workspace creation date or
workspace join date to ensure consistent ordering, or modify the query logic to
detect and return a conflict/disambiguation error when the same ticket
key-number pair exists in multiple workspaces that the user can access,
preventing non-deterministic behavior in the response.
In `@internal-docs/kanban/unification-v2.md`:
- Around line 64-72: The webhook schema reference in the table incorrectly uses
`board_document_id` keyed to the board's `document_id`, which would cause the
webhook scope to change when files are renamed or moved. Replace the webhook
entry `webhooks.board_document_id` with `webhooks.board_ref` and change its
relationship to reference the board's `board_ref` (from frontmatter), not the
`document_id`, to maintain consistent scope across file operations and align
with the actual migration contract. Apply the same change to the additional
occurrence mentioned in lines 112-119.
- Around line 62-72: The final sentence states that ticket strings will change
with board key renames, which contradicts the requirement that allocated ticket
IDs like OCCSV-3371 must remain stable after board-key renames. Correct this
statement to clarify that allocated ticket IDs stored in the card_tickets table
are immutable snapshots of ticket_key plus number, and therefore remain valid
and unchanged regardless of subsequent board-key renames. Explain that the
document_id foreign key relationship in card_tickets ensures ticket stability is
maintained independent of any board-level changes.
In `@services/jtype-cli/src/kanban.rs`:
- Around line 82-99: The code currently accepts any status value without
verifying it exists as a valid column in the board configuration, which can
result in orphaned cards. Before writing the card frontmatter in the
build_card_content function call and in any move/set operations, add validation
to confirm that the provided status is a valid column key within the board
object b. Retrieve the board's column definitions (likely from the board schema
or configuration) and check that the status parameter matches one of the valid
columns. If the status is invalid, return an appropriate error message to the
user indicating the valid column options.
- Around line 87-100: The `rel` variable is constructed solely from
`slugify(title)`, which means cards with identical titles will target the same
markdown file path and overwrite each other. Before calling `save_note_local`,
check if a file already exists at the calculated `rel` path, and if a collision
is detected, append a numeric suffix to the slug to generate a unique path. This
prevents overwriting existing cards while maintaining the slugified title as the
base of the filename.
In `@services/jtype-cli/src/main.rs`:
- Around line 332-347: Card mutation operations (move and set) should not create
a vault directory if one does not exist, as these commands require existing
board and card files. Replace the call to vault_or_init with require_vault in
the CardCmd::Move handler and the CardCmd::Set handler. Keep the CardCmd::Create
handler unchanged as it is a bootstrap operation that should initialize the
vault if needed. Update both move_card_local and set_card_local call sites to
use require_vault instead of vault_or_init.
In `@services/jtype-web/frontend/src/pages/WebBoardView.tsx`:
- Around line 501-504: The button with `absolute` positioning and the className
containing "bottom-4 right-4" needs to be anchored to the board container, but
the parent wrapper at line 485 lacks positioning, causing it to anchor to an
outer layout instead. Add `relative` positioning to the wrapper container at
line 485 to establish it as the positioning context for the absolutely
positioned webhooks button, ensuring the button stays within the board view
boundaries.
In `@services/jtype-web/migrations/0021_card_tickets.up.sql`:
- Line 22: The UNIQUE KEY constraint uq_card_tickets_ref on the card_tickets
table starts with workspace_id, which means database queries filtering by just
(ticket_key, number) without workspace_id will not use this index and will
require expensive table scans. Add a separate, non-unique index on the
card_tickets table with the columns (ticket_key, number) to efficiently support
the workspace-agnostic browse flow queries. Place this new index definition in
the same migration file after the unique constraint definition.
In `@services/jtype-web/src/handlers/document.rs`:
- Around line 314-317: The comment for the persist document version function
incorrectly claims that all write paths including conflict resolution notify via
webhook, but the actual implementation in resolve_conflict and other operations
shows this is not true. Update the comment in document.rs to accurately reflect
that only certain paths (specifically save_document_version, not
save_merged_document) fire the kanban webhook, and clarify that conflict
resolution strategies like accept_local, accept_cloud, and manual_merge in
sync.rs do not trigger the webhook notification because they call
save_merged_document directly instead of save_document_version. Also document
that other operations like trash/restore and publish/unpublish similarly bypass
the webhook.
In `@services/jtype-web/src/handlers/tickets.rs`:
- Around line 225-232: The SQL query in the sqlx::query for browsing tickets
uses LIMIT 1 without filtering by a specific workspace_id or ordering
deterministically, causing nondeterministic results when a user belongs to
multiple workspaces that contain the same ticket_key and number combination. Add
either a workspace_id parameter to the WHERE clause to filter for a specific
workspace, or add an ORDER BY clause with a deterministic ordering to ensure
consistent results, and handle the case where multiple matching tickets exist
across workspaces by either requiring workspace context in the request or
explicitly handling the ambiguity in the application logic.
- Around line 56-79: The resolve_document_id function accepts any document type
in the workspace, which allows editors to allocate tickets to non-card documents
like `.board` files or ordinary notes. After calling resolve_document_id in the
allocate function, add validation to verify that the resolved document is a
`.md` card with `board:` frontmatter before proceeding with ticket allocation.
You can query the documents table to check the file type and frontmatter
metadata, returning an appropriate error if the document does not meet the card
criteria.
In `@services/jtype-web/src/mcp/kanban_tools.rs`:
- Around line 180-188: The create_card function always uses the same slug-based
path format {dir}/{slug(title)}.md for saving cards, which means duplicate
titles that produce identical slugs will silently overwrite existing card files.
Implement collision detection before the api_post call to check if a file
already exists at the target path, and if it does, append a numeric suffix or
similar unique identifier to the slug (e.g., {dir}/{slug(title)}_1.md) to
prevent overwrites. This ensures that each card creation either creates a new
file or fails gracefully rather than silently clobbering existing cards.
- Around line 229-233: The issue is that the clear field logic in the
update_card function is unreachable because the opt function drops empty strings
upstream before the v.is_empty() check can ever be true, preventing fields like
assignee and due from being cleared. To fix this, modify the logic to
differentiate between a field argument that was provided but is empty (which
should clear the field) versus a field argument that was not provided at all
(which should be skipped). Instead of relying on opt to return
Some(empty_string), you need to check if the field key exists in args separately
from whether its value is empty, so that empty strings can be passed as None to
set_frontmatter_field to clear the fields as documented.
In `@services/jtype-web/src/tasks/webhook_delivery.rs`:
- Around line 105-127: When delivery is blocked by the SSRF check in the
ssrf_blocked conditional block, the code updates the webhook_deliveries table
but fails to also update the webhooks table with metadata fields
last_delivery_at and last_status, causing webhooks to display stale status
information. After the webhook_deliveries UPDATE query executes in the
ssrf_blocked branch, add a second query to update the webhooks table to set
last_status to the same status value (either "dead" or "failed" based on the
dead flag) and last_delivery_at to the current timestamp, ensuring webhook
metadata remains consistent with the delivery state.
In `@services/jtype-web/tests/mcp_tests.rs`:
- Around line 117-119: The test coverage currently only validates the notes
catalog at the `/mcp` endpoint but does not cover the new Kanban catalog at
`/mcp/kanban`. Add a new test case that verifies the Kanban catalog exposes
board and card tools (such as board-related and card-related operations).
Additionally, add a negative assertion in the existing test (around the for loop
that checks the expected list with "list_workspaces", "list_notes", etc.) to
verify that `/mcp` does not expose board or card tools, ensuring the tools are
properly isolated to their respective endpoints.
In `@shared/lib/board.ts`:
- Around line 220-221: The TAG_COLORS constant exports hardcoded hex color
values, which violates the shared code styling guidelines that require semantic
token usage. Replace the hardcoded hex values in the TAG_COLORS array with
semantic token references from shared/styles/tokens.css. Update each color in
the array to reference the appropriate semantic token variable instead of the
raw hex value to ensure theming compliance.
In `@src/lib/types.ts`:
- Around line 28-30: The labels field defines an inline object shape { label:
string; color?: string | null } which violates the project's typing guidelines.
Extract this object shape into a separate interface (such as BoardLabel or
Label) with properties for label and color, then update the labels field to
reference this new interface as an array type instead of using the inline object
definition.
---
Outside diff comments:
In `@services/jtype-web/src/handlers/mod.rs`:
- Around line 5-20: Create three new integration test files in
services/jtype-web/tests/ named comments_tests.rs, tickets_tests.rs, and
webhooks_tests.rs to test the handler modules exported in mod.rs. Each test file
should contain test cases covering all endpoints defined in the respective
handler modules: comments_tests.rs should test list_comments, create_comment,
and delete_comment endpoints; tickets_tests.rs should test allocate,
list_tickets, and resolve_ticket endpoints; webhooks_tests.rs should test
browse, list_webhooks, create_webhook, and delete_webhook endpoints. Each test
should use common::setup().await to initialize the test harness,
tower::ServiceExt::oneshot to send requests to the service, and common::uid()
and common::wname() for test data generation as needed.
In `@services/jtype-web/src/handlers/webhooks.rs`:
- Around line 107-126: The is_blocked_v4 and is_blocked_v6 functions do not
block all non-global IP ranges, allowing CGNAT ranges like 100.64.0.0/10, IPv4
multicast ranges, IPv6 multicast ranges, and IPv6 documentation ranges through.
Add additional checks to is_blocked_v4 to block IPv4 multicast (224.0.0.0/4) and
CGNAT (100.64.0.0/10) ranges by checking the first octet and appropriate bits,
and add checks to is_blocked_v6 to block IPv6 multicast (ff00::/8) and
documentation ranges (2001:db8::/32) by examining the segments, or alternatively
use the is_global() method if available to return true only for genuinely global
addresses.
---
Nitpick comments:
In `@shared/components/board/BoardSurface.tsx`:
- Around line 720-724: The ticket badge in the span element uses hardcoded
Tailwind color classes (bg-stone-100 and text-stone-500) instead of semantic
color tokens. Replace these palette-specific utilities with semantic token
classes from shared/styles/tokens.css following the shared component guidelines.
Identify the span element displaying the card.ticket and swap the hardcoded
color classes (bg-stone-100 for background and text-stone-500 for text) with the
appropriate semantic token classes (e.g., bg-brand-soft, text-brand patterns)
that maintain consistency across desktop and web theming.
🪄 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: 67dc1ff2-821a-4d3f-89be-3593c36997c1
⛔ Files ignored due to path filters (1)
services/jtype-web/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (55)
internal-docs/doc-kanban-unification/design.mdinternal-docs/kanban/design.mdinternal-docs/kanban/gaps-and-roadmap.mdinternal-docs/kanban/next-features-design.mdinternal-docs/kanban/ticket-links.mdinternal-docs/kanban/unification-v2.mdinternal-docs/web-board-alignment/design.mdservices/jtype-cli/src/client.rsservices/jtype-cli/src/kanban.rsservices/jtype-cli/src/main.rsservices/jtype-cli/src/mcpstdio.rsservices/jtype-cli/tests/e2e.shservices/jtype-core/src/lib.rsservices/jtype-web/Cargo.tomlservices/jtype-web/frontend/src/api.tsservices/jtype-web/frontend/src/main.tsxservices/jtype-web/frontend/src/pages/Kanban.tsxservices/jtype-web/frontend/src/pages/WebBoardView.tsxservices/jtype-web/frontend/src/pages/Workspace.tsxservices/jtype-web/migrations/0018_drop_kanban.down.sqlservices/jtype-web/migrations/0018_drop_kanban.up.sqlservices/jtype-web/migrations/0019_card_comments.down.sqlservices/jtype-web/migrations/0019_card_comments.up.sqlservices/jtype-web/migrations/0020_webhooks.down.sqlservices/jtype-web/migrations/0020_webhooks.up.sqlservices/jtype-web/migrations/0021_card_tickets.down.sqlservices/jtype-web/migrations/0021_card_tickets.up.sqlservices/jtype-web/src/db/migrations.rsservices/jtype-web/src/db/models.rsservices/jtype-web/src/handlers/comments.rsservices/jtype-web/src/handlers/document.rsservices/jtype-web/src/handlers/kanban/board.rsservices/jtype-web/src/handlers/kanban/card.rsservices/jtype-web/src/handlers/kanban/column.rsservices/jtype-web/src/handlers/kanban/label.rsservices/jtype-web/src/handlers/kanban/mod.rsservices/jtype-web/src/handlers/mod.rsservices/jtype-web/src/handlers/tickets.rsservices/jtype-web/src/handlers/webhooks.rsservices/jtype-web/src/hub.rsservices/jtype-web/src/lib.rsservices/jtype-web/src/mcp/kanban_tools.rsservices/jtype-web/src/mcp/mod.rsservices/jtype-web/src/mcp/tools.rsservices/jtype-web/src/tasks/cleanup_trash.rsservices/jtype-web/src/tasks/mod.rsservices/jtype-web/src/tasks/webhook_delivery.rsservices/jtype-web/tests/kanban_e2e_tests.rsservices/jtype-web/tests/kanban_tests.rsservices/jtype-web/tests/mcp_tests.rsshared/components/board/BoardSurface.tsxshared/components/board/controls.tsxshared/lib/board.tssrc/components/BoardView.tsxsrc/lib/types.ts
💤 Files with no reviewable changes (9)
- services/jtype-web/src/handlers/kanban/board.rs
- services/jtype-web/tests/kanban_e2e_tests.rs
- services/jtype-web/src/handlers/kanban/mod.rs
- services/jtype-web/src/handlers/kanban/card.rs
- services/jtype-web/frontend/src/pages/Kanban.tsx
- services/jtype-web/src/handlers/kanban/column.rs
- services/jtype-web/tests/kanban_tests.rs
- services/jtype-web/src/handlers/kanban/label.rs
- services/jtype-web/src/hub.rs
Comment on lines
+220
to
+221
| /** Palette for auto-assigned tag colors (deterministic by label). */ | ||
| export const TAG_COLORS = ["#ef4444", "#f59e0b", "#eab308", "#22c55e", "#14b8a6", "#0ea5e9", "#6366f1", "#a855f7", "#ec4899", "#78716c"]; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace hardcoded tag palette hex values with semantic tokens.
TAG_COLORS introduces hardcoded hex values in shared code. Please switch this palette to token-backed semantic values (from shared/styles/tokens.css) so shared theming stays policy-compliant.
As per coding guidelines shared/**/*.{ts,tsx,css}: shared code must use semantic styling tokens and “never hardcode hex values”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shared/lib/board.ts` around lines 220 - 221, The TAG_COLORS constant exports
hardcoded hex color values, which violates the shared code styling guidelines
that require semantic token usage. Replace the hardcoded hex values in the
TAG_COLORS array with semantic token references from shared/styles/tokens.css.
Update each color in the array to reference the appropriate semantic token
variable instead of the raw hex value to ensure theming compliance.
Source: Coding guidelines
Comment on lines
+28
to
+30
| /** Board-level label definitions giving tags an explicit color (else auto-colored). */ | ||
| labels?: { label: string; color?: string | null }[]; | ||
| /** Board ticket-id prefix (e.g. OCCSV) for /browse links; ticket numbers are cloud-only. */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use an interface for the new label object shape.
The added labels field introduces a new inline object shape; this should be an interface per project typing rules.
Proposed refactor
+export interface BoardLabelDef {
+ label: string;
+ color?: string | null;
+}
+
export type BoardConfig = {
@@
- labels?: { label: string; color?: string | null }[];
+ labels?: BoardLabelDef[];As per coding guidelines src/**/*.{ts,tsx}: “Use interface for defining object shapes in TypeScript frontend code.”
📝 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
| /** Board-level label definitions giving tags an explicit color (else auto-colored). */ | |
| labels?: { label: string; color?: string | null }[]; | |
| /** Board ticket-id prefix (e.g. OCCSV) for /browse links; ticket numbers are cloud-only. */ | |
| export interface BoardLabelDef { | |
| label: string; | |
| color?: string | null; | |
| } | |
| export type BoardConfig = { | |
| /** Board-level label definitions giving tags an explicit color (else auto-colored). */ | |
| labels?: BoardLabelDef[]; | |
| /** Board ticket-id prefix (e.g. OCCSV) for /browse links; ticket numbers are cloud-only. */ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/types.ts` around lines 28 - 30, The labels field defines an inline
object shape { label: string; color?: string | null } which violates the
project's typing guidelines. Extract this object shape into a separate interface
(such as BoardLabel or Label) with properties for label and color, then update
the labels field to reference this new interface as an array type instead of
using the inline object definition.
Source: Coding guidelines
Workspace-scope ticket resolution and drop the ambiguous workspace-agnostic /browse handler+route; resolve only via the workspace-scoped route (frontend TicketRedirect). Restrict ticket allocation to .md cards with board: frontmatter (same card test as fire_card_webhook). Fire the card webhook after conflict resolution's merged-save (accept_local/accept_cloud/manual_merge) — other write paths unchanged. Also from the review triage: SSRF guard now blocks CGNAT/multicast/reserved/ benchmark v4 and IPv6 multicast/doc ranges (is_global is unstable on 1.89, so bitmasks); webhook delivery keeps webhooks.last_status in step on blocked deliveries; CLI/MCP avoid slug-collision card overwrites; CLI validates column keys and uses require_vault for card mutations; MCP update_card can clear fields; add /mcp/kanban catalog tests; align ticket-links/unification-v2 docs with the implemented schema (board_ref, VARCHAR(16), ticket-id stability). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jtype-web depends on the sibling crate via `jtype-core = { path = "../jtype-core" }`,
added when the kanban CLI/MCP was rebuilt on jtype-core. The Dockerfile builder
(WORKDIR /app) only copied jtype-web's own Cargo.toml+src, so the path resolved
to /jtype-core which was never in the image — `cargo build --release` failed at
dependency resolution. Copy services/jtype-core to /jtype-core (its target/ is
already stripped by .dockerignore). Verified: image builds and the container
boots, serves the SPA, and registers a user.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/jtype-web/src/handlers/tickets.rs (1)
89-178: 📐 Maintainability & Code Quality | 🟠 MajorAdd test coverage for the ticket API endpoints.
The new ticket endpoints (
POST /api/v1/workspaces/:workspace_id/tickets/allocate,GET /api/v1/workspaces/:workspace_id/tickets, andGET /api/v1/workspaces/:workspace_id/tickets/:ticket) are registered in the router but have no corresponding tests. Per the coding guidelines forservices/jtype-web/src/handlers/**/*.rs, new endpoints must include test cases. Createservices/jtype-web/tests/tickets_tests.rswith integration tests covering allocate, list, and resolve operations using the established web-service test conventions (common::setup().await,common::req(), etc.).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/jtype-web/src/handlers/tickets.rs` around lines 89 - 178, The ticket API endpoints including the allocate function and associated list/resolve operations lack corresponding test coverage. Create a new integration test file at services/jtype-web/tests/tickets_tests.rs that uses the established testing conventions from the project (such as common::setup().await and common::req()) to add comprehensive test cases covering the allocate endpoint success and failure scenarios (including concurrent allocation conflicts), the list endpoint for retrieving tickets, and the resolve endpoint for looking up tickets by key. Ensure tests validate both happy paths and edge cases such as workspace permission validation and idempotent behavior of the allocate function.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@services/jtype-web/src/handlers/tickets.rs`:
- Around line 89-178: The ticket API endpoints including the allocate function
and associated list/resolve operations lack corresponding test coverage. Create
a new integration test file at services/jtype-web/tests/tickets_tests.rs that
uses the established testing conventions from the project (such as
common::setup().await and common::req()) to add comprehensive test cases
covering the allocate endpoint success and failure scenarios (including
concurrent allocation conflicts), the list endpoint for retrieving tickets, and
the resolve endpoint for looking up tickets by key. Ensure tests validate both
happy paths and edge cases such as workspace permission validation and
idempotent behavior of the allocate function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da23bfd1-a6d3-4e8f-9779-32fad442c884
📒 Files selected for processing (19)
internal-docs/kanban/ticket-links.mdinternal-docs/kanban/unification-v2.mdservices/jtype-cli/src/kanban.rsservices/jtype-cli/src/main.rsservices/jtype-web/Dockerfileservices/jtype-web/frontend/src/api.tsservices/jtype-web/frontend/src/main.tsxservices/jtype-web/frontend/src/pages/WebBoardView.tsxservices/jtype-web/frontend/src/pages/Workspace.tsxservices/jtype-web/src/handlers/document.rsservices/jtype-web/src/handlers/sync.rsservices/jtype-web/src/handlers/tickets.rsservices/jtype-web/src/handlers/webhooks.rsservices/jtype-web/src/lib.rsservices/jtype-web/src/mcp/kanban_tools.rsservices/jtype-web/src/tasks/webhook_delivery.rsservices/jtype-web/tests/mcp_tests.rsshared/lib/board.tssrc/lib/types.ts
✅ Files skipped from review due to trivial changes (1)
- internal-docs/kanban/unification-v2.md
🚧 Files skipped from review as they are similar to previous changes (11)
- services/jtype-web/frontend/src/pages/Workspace.tsx
- src/lib/types.ts
- services/jtype-web/src/lib.rs
- shared/lib/board.ts
- services/jtype-web/src/handlers/document.rs
- services/jtype-web/src/mcp/kanban_tools.rs
- services/jtype-web/frontend/src/pages/WebBoardView.tsx
- services/jtype-cli/src/kanban.rs
- services/jtype-web/src/handlers/webhooks.rs
- services/jtype-web/src/tasks/webhook_delivery.rs
- services/jtype-cli/src/main.rs
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
1 participant
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.
What & why
Retires the orphaned cloud-DB kanban (
/kanban,kanban_*tables — unreachable from the UI and not synced to desktop) and converges everything on the document-backed board: cards are.mdnotes, boards are.boardviews, all synced through thedocumentstable. Every relational feature is rebuilt fresh, keyed bydocuments.id.Changes
Retire ③ (cloud-DB board)
Kanban.tsx+/kanbanroute,handlers/kanban/*,kanban_*models, hub kanban events,kanban_tests/e2e. Migration 0018 drops all 9kanban_*tables.Kanban MCP + CLI (rebuilt on documents)
POST /mcp/kanban(mcp/kanban_tools.rs,ServerKind).jtype board/cardCLI onjtype-core(list_boards,set_frontmatter_field);jtype mcp-stdio --kanbanbridge.Feature parity on the shared
BoardSurfaceresolveTags/autoTagColor), member assignee (listMembers), per-card activity timeline (document_versions+ author username).Comments / webhooks / ticket-links
card_comments(FKdocuments.id).webhooks+ delivery worker, re-homed intosave_document_versionso REST + desktop sync push + conflict + live all fire; SSRF-hardened (IPv4-mapped/fe80::IPv6 + delivery-time DNS-rebinding re-check).card_tickets+board_sequencesfor Jira-style/browse/OCCSV-3371(atomicLAST_INSERT_IDallocator, idempotent per document).Adversarial review
Three review passes (34 agents) over the diff; real findings fixed:
LAST_INSERT_ID→CAST AS SIGNED(was 500ing every allocation), idempotent concurrent allocate, webhook SSRF gaps + the sync-push trigger gap.Test plan
jtype-web/jtype-core/jtype-clicargo check --tests— 0 warningstsc+vite build; desktoptsc🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Architecture
.board) + card notes (.md) with cloud-local interoperability.Removals