Conversation
…t cleanup - Remove 4-tab layout (Assets/Push/Setup/Roles), flatten to single page - Add per-asset action buttons: Install, Update (via sx remove+add), Remove - Add new POST /api/teams/update-asset endpoint for proper version updates - Inline push panel with "Push to:" scope selector - Add Reconfigure button to repository setup section - Remove Roles feature entirely (TeamsRolesTab, roles endpoints, hook methods) - Rename all Vault* types to Teams* (VaultStatus→TeamsStatusResponse, etc.) - Rename vaultUrl→repoUrl across full stack (JSON wire format + consumers) - Clean up vault references in settings.json, installer, rules, README - Update navigation: /vault→/teams route, sidebar, command palette, shortcuts
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Claude finished @maxritter's task in 2m 9s —— View job Code Review
|
WalkthroughThis pull request performs a comprehensive migration from "Vault" to "Teams" terminology across the application. The backend replaces VaultRoutes with a new TeamsRoutes implementation providing expanded API endpoints for asset management. Frontend routing, navigation, and UI components are updated to use Teams, including new setup and push panels. Vault-specific components and tests are removed while Teams equivalents are added. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (7)
README.md-315-315 (1)
315-315:⚠️ Potential issue | 🟡 MinorComplete the README terminology migration to avoid user confusion.
Good update here, but the README still references legacy Vault terms elsewhere (for example: Line 158
/vault, Line 186Vaultview, Line 517/vault). If/teamsis now the canonical route/feature name, these should be updated in the same pass so docs match product behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 315, The README still uses legacy "Vault" terminology and routes (e.g., occurrences of "/vault", "Vault" view, and the legacy file reference `team-vault.md`) which should be migrated to the new canonical "teams" naming; search for and replace all references of "/vault" with "/teams", rename mentions of "Vault" view to "Teams" view (preserving context/capitalization), and update or remove `team-vault.md` references to the new `teams` documentation or filename so the docs consistently reflect the product naming.console/src/ui/viewer/components/CommandPalette.tsx-124-132 (1)
124-132:⚠️ Potential issue | 🟡 MinorArrow-key navigation breaks when no commands are available.
When
filteredCommands.length === 0, modulo operations setselectedIndextoNaN.🔧 Suggested guard
const handleKeyDown = (e: React.KeyboardEvent) => { + if ( + filteredCommands.length === 0 && + (e.key === "ArrowDown" || e.key === "ArrowUp" || e.key === "Enter") + ) { + e.preventDefault(); + return; + } + switch (e.key) { case "ArrowDown": e.preventDefault(); setSelectedIndex((i) => (i + 1) % filteredCommands.length); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/src/ui/viewer/components/CommandPalette.tsx` around lines 124 - 132, Arrow-key handlers in CommandPalette.tsx compute new selected index using modulo which yields NaN when filteredCommands.length === 0; add a guard in the ArrowDown and ArrowUp cases (or before invoking setSelectedIndex) to no-op when filteredCommands.length === 0 so setSelectedIndex is not called with invalid math, keeping the current selection unchanged (or explicitly set to a safe default like -1/0 if that state is handled elsewhere).console/src/ui/viewer/views/Teams/TeamsSummaryCards.tsx-49-54 (1)
49-54:⚠️ Potential issue | 🟡 MinorKeep the stat description aligned with the displayed metric.
When title/value include “Other”, the description “Slash commands” is misleading.
💡 Suggested tweak
- <div className="stat-desc">Slash commands</div> + <div className="stat-desc"> + {other > 0 ? "Slash commands + other asset types" : "Slash commands"} + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/src/ui/viewer/views/Teams/TeamsSummaryCards.tsx` around lines 49 - 54, The stat description in TeamsSummaryCards.tsx (the elements with classNames stat-title, stat-value, stat-desc that display commands and other via variables commands and other) can become misleading when other > 0; update the rendering logic for the stat-desc so it conditionally matches the displayed metric (e.g., when other > 0 show a description like "Slash commands and other" or similar, otherwise keep "Slash commands") so the description aligns with the title/value.console/tests/ui/teams-view.test.ts-116-116 (1)
116-116:⚠️ Potential issue | 🟡 MinorUpdate stale test naming to Teams terminology.
The test title still says “vault assets”; rename it to “team assets” for accurate behavior description.
As per coding guidelines, "Review test code briefly. Focus on: Clear test names that describe behavior".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/tests/ui/teams-view.test.ts` at line 116, Rename the test title string in teams-view.test.ts from "renders install button for uninstalled vault assets" to "renders install button for uninstalled team assets" so the test name matches the Teams terminology; locate the it(...) call in the file (the test case starting with it("renders install button for uninstalled vault assets", ...) and update only the descriptive string, leaving the test body and assertions unchanged.console/src/ui/viewer/views/Teams/index.tsx-41-42 (1)
41-42:⚠️ Potential issue | 🟡 MinorAvoid gating with
nulltier before license load completes.Because
useLicense().isLoadingisn’t used,TeamGatereceivesnullduring fetch and can briefly render the locked/upgrade state for eligible users. Gate these sections only after license loading settles.Also applies to: 182-190, 210-220, 254-256
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/src/ui/viewer/views/Teams/index.tsx` around lines 41 - 42, The Team gating logic is using license which can be null while the license is still loading; update the code that calls useLicense() to also read isLoading and defer rendering any TeamGate checks until loading settles (for example, return a loader or skip rendering TeamGate when isLoading is true), and then pass the resolved license/tier (or undefined) into TeamGate instead of null so eligible users don't briefly see locked UI; apply the same change to all TeamGate usages referenced (the blocks around where useLicense() is read and TeamGate is rendered).console/src/ui/viewer/views/Dashboard/TeamsStatus.tsx-68-71 (1)
68-71:⚠️ Potential issue | 🟡 MinorUse a composite key for availability counts (name-only matching is lossy).
Current logic matches installed/catalog entries by
nameonly. If two assets share a name across types,availableCountis incorrect (Line [68]-Line [71]). Match ontype + nameto avoid false exclusions.Proposed fix
- const installedNames = new Set(assets.map((a) => a.name)); + const installedKeys = new Set(assets.map((a) => `${a.type}:${a.name}`)); const availableCount = catalog.filter( - (c) => !installedNames.has(c.name), + (c) => !installedKeys.has(`${c.type}:${c.name}`), ).length;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/src/ui/viewer/views/Dashboard/TeamsStatus.tsx` around lines 68 - 71, The availability check in TeamsStatus.tsx builds installedNames from assets using name-only and computes availableCount by filtering catalog by name, which breaks when assets share names across types; update the logic to use a composite key (e.g., `${type}::${name}`) for both installedNames and the catalog filter so matching is done on type+name (ensure you reference the installedNames set and availableCount calculation), and handle possible missing/undefined type values consistently when building the composite keys.console/src/ui/viewer/views/Teams/TeamsSetupTab.tsx-66-71 (1)
66-71:⚠️ Potential issue | 🟡 MinorReplace stale “vault” placeholders in Teams UI copy.
Lines [68]-[70] still show legacy vault wording (
team-vault.git,/path/to/vault). This is user-facing and inconsistent with the Teams rename.Proposed fix
placeholder={ repoType === "git" - ? "git@github.com:org/team-vault.git" + ? "git@github.com:org/team-repo.git" : repoType === "path" - ? "/path/to/vault" + ? "/path/to/team-repo" : "https://skills.new/..." }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/src/ui/viewer/views/Teams/TeamsSetupTab.tsx` around lines 66 - 71, In TeamsSetupTab (the placeholder ternary using repoType), replace legacy "vault" wording in the user-facing placeholders: change "git@github.com:org/team-vault.git" to a non-vault name (e.g., "git@github.com:org/team-repo.git") and change "/path/to/vault" to a matching local path (e.g., "/path/to/team-repo" or "/path/to/teams") so the placeholders reflect the Teams rename and remain consistent with the UI copy.
🧹 Nitpick comments (4)
console/tests/hooks/useSettings.test.ts (1)
47-53: Strengthen this test to assert the exact command key set.Line 47 says “all six commands,” but the current assertions allow additional unexpected keys (except
vault). Assert exact keys to catch accidental defaults drift.Proposed test tightening
it('DEFAULT_SETTINGS has all six commands', async () => { const { DEFAULT_SETTINGS } = await import('../../src/ui/viewer/hooks/useSettings.js'); const expected = ['spec', 'spec-plan', 'spec-implement', 'spec-verify', 'sync', 'learn']; - for (const cmd of expected) { - expect(DEFAULT_SETTINGS.commands[cmd]).toBeDefined(); - } - expect(DEFAULT_SETTINGS.commands['vault']).toBeUndefined(); + expect(Object.keys(DEFAULT_SETTINGS.commands).sort()).toEqual(expected.sort()); });As per coding guidelines, "
**/tests/**: Review test code briefly. Focus on: Test coverage for the feature being tested."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/tests/hooks/useSettings.test.ts` around lines 47 - 53, The test currently checks only presence of six expected commands and absence of 'vault', so update the assertion to verify the exact set of keys in DEFAULT_SETTINGS.commands to prevent accidental additions; in the test for DEFAULT_SETTINGS (in useSettings.test.ts) import DEFAULT_SETTINGS and replace the per-key checks with a single assertion that Object.keys(DEFAULT_SETTINGS.commands) (sorted) strictly equals the expected array (sorted) — referencing DEFAULT_SETTINGS and its commands property to locate the code to change.console/tests/worker/teams-routes.test.ts (1)
168-190: Discover test should avoid real fs/git dependencies and sleep-based waiting.Mock filesystem/git interactions and
awaitthe handler directly to keep tests deterministic and non-flaky.As per coding guidelines, "`**/tests/**`: Review test code briefly. Focus on: Proper mocking of external dependencies".✅ Minimal deterministic pattern
- discoverHandler({}, fakeRes); - await new Promise((r) => setTimeout(r, 50)); + await discoverHandler({}, fakeRes);Also applies to: 184-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/tests/worker/teams-routes.test.ts` around lines 168 - 190, The test calls TeamsRoutes.setupRoutes then invokes the discover handler and uses a sleep to wait; instead, mock external FS/Git interactions used by TeamsRoutes (e.g., any fs, git, or repo helpers it depends on) using jest mocks/spies and call the route handler as an async function so you can await it directly; locate the route via TeamsRoutes.setupRoutes and the discoverHandler reference, provide a fakeReq/fakeRes that returns a Promise from json/status (or make the handler return a Promise) and then await discoverHandler(fakeReq, fakeRes) instead of using setTimeout to make the test deterministic and avoid touching the real filesystem or git.console/tests/ui/teams-navigation.test.ts (1)
11-49: Prefer module/behavior assertions over file-content matching for routing wiring.These checks are mostly string scans, which are brittle and can miss real wiring regressions. Prefer asserting exported route/nav/command data structures directly where possible.
As per coding guidelines, "Review test code briefly. Focus on: Test coverage for the feature being tested".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/tests/ui/teams-navigation.test.ts` around lines 11 - 49, Replace brittle fs string-search assertions with direct module imports and assertions against the exported route/nav/command structures: import the routes export from the module that defines App.tsx routes (e.g., the exported routes array or a getRoutes function) and assert there is an entry with component/name "TeamsView" and path "/teams"; import the nav items export from SidebarNav (e.g., navItems or sidebarConfig) and assert an item with label "Teams" has icon "users" and href or to "/teams"; import the commands export from CommandPalette (e.g., commands or paletteItems) and assert a command labeled "Go to Teams" exists with keybinding "G V" and a navigate handler or route pointing to "/teams". Ensure tests use the exported symbols (routes, navItems, commands) rather than reading file text so wiring is verified at runtime.console/src/ui/viewer/views/Teams/index.tsx (1)
12-19: DeduplicateformatRepoUrlinto a shared utility.This formatter is now repeated across Teams views, which increases drift risk for URL display behavior. Extract to a shared helper and import it in both places.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/src/ui/viewer/views/Teams/index.tsx` around lines 12 - 19, The formatRepoUrl function is duplicated; extract it into a shared helper (e.g., export function formatRepoUrl(url: string): string in a new/util file) and replace the local definition in Teams view(s) with an import of that helper; update the callers in Teams/index.tsx (and any other Teams view files using formatRepoUrl) to import the new shared function and remove the local implementation so URL formatting is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@console/src/services/worker/http/routes/TeamsRoutes.ts`:
- Around line 457-468: The update-asset handler (handleUpdateAsset) mutates the
lockfile and runs installs without synchronizing with concurrent installs; guard
the critical section by checking and setting the shared flag _isInstalling: at
the start of the handler, if (this._isInstalling) return a 409/conflict
response, otherwise set this._isInstalling = true before performing lockfile
mutation and the install, and ensure you clear it in a finally block so it’s
always reset; apply the same pattern to the other install/update handler
referenced around lines 495-499 (the other route handler that mutates the
lockfile) so both use the same check/set/try...finally/unset sequence and avoid
race conditions.
- Around line 320-324: The catch block in the Teams push handler currently
returns a 200 OK with an error payload; update the error handling to send an
appropriate non-2xx HTTP status (e.g., res.status(500)) before returning the
JSON error, and include the same message and logger.error usage (see the catch
that defines const message = (error as Error).message and calls
logger.error("HTTP", "Teams push failed", { name }, error as Error)); apply the
identical change to the catch blocks in handleRemove, handleInit, and
handleUpdateAsset so failures return a non-2xx status with the same error JSON
and logging behavior.
- Around line 470-494: The current remove-then-add sequence can leave the
lockfile degraded if add/install fails; change the flow in the method that calls
runSxCommand so you add the new asset before removing the old (build addArgs and
call runSxCommand(addArgs, PUSH_TIMEOUT_MS) first), and only on successful add
call runSxCommand([...sxPath, "remove", name, "--version",
String(currentVersion), "--yes"], REMOVE_TIMEOUT_MS) to drop the old entry;
additionally wrap both operations in try/catch and on any failure attempt a
compensating rollback by calling runSxCommand([...sxPath, "add", name,
"--version", String(currentVersion), "--yes"], PUSH_TIMEOUT_MS) to restore the
previous version (use currentVersion, addArgs, runSxCommand, REMOVE_TIMEOUT_MS,
PUSH_TIMEOUT_MS to locate code).
- Around line 282-290: The current prefix check on resolvedSource can be
bypassed via symlinks; change validation to compare real filesystem paths: call
fs.promises.realpath (or fs.realpathSync) for projectRoot and resolvedSource
(referencing projectRoot and resolvedSource variables and path.resolve usage)
and verify the resolvedSourceRealpath equals projectRootRealpath or
startsWith(projectRootRealpath + path.sep) before accepting; return the same 400
error on failure. Ensure to handle realpath errors (treat as invalid) and
preserve use of process.env.CLAUDE_PROJECT_ROOT when computing
projectRootRealpath.
In `@console/src/ui/viewer/components/TeamGate.tsx`:
- Around line 16-18: The overlay wrapper currently uses pointer-events-none
which doesn't prevent keyboard focus; update the wrapper div in TeamGate (the
element rendering {children}) to remove keyboard access by adding accessibility
attributes: set aria-hidden="true" and add the inert attribute (and fallback to
tabIndex={-1} if inert not supported) on that div so underlying interactive
controls are excluded from the accessibility tree and cannot receive keyboard
focus; ensure the same element keeps the visual classes (opacity-30 blur-[2px]
select-none) while adding these attributes.
In `@console/src/ui/viewer/hooks/useStats.ts`:
- Around line 178-183: The loadTeamsStatusData callback currently assigns any
JSON response to teamsStatusData; update loadTeamsStatusData to first verify the
fetch response with if (!res.ok) return or set a safe fallback, then parse JSON
and validate its shape (e.g., confirm expected keys/array via Array.isArray or
typeof checks) before calling setTeamsStatusData; on invalid shape or parse
errors, set a safe default (empty array/object) and handle/log the error in the
catch block so setTeamsStatusData is never called with unexpected payloads.
In `@console/src/ui/viewer/hooks/useTeams.ts`:
- Around line 241-317: The issue: successful mutations do not consistently
refresh Teams state—modify initTeams, pushAsset, and removeAsset to call and
await fetchStatus() when the API returns success (like updateAsset does) so the
UI isn't stale; also update the useCallback dependency arrays for pushAsset and
removeAsset to include fetchStatus so closures reference the latest function.
Ensure each of initTeams, pushAsset, and removeAsset awaits fetchStatus() after
detecting data.success (or after res.ok and parsed success) before returning.
In `@console/src/ui/viewer/views/Teams/TeamsAssetTable.tsx`:
- Around line 163-174: The remove flow in handleRemoveClick can leave isRemoving
true if onRemove rejects; wrap the await onRemove(asset.name) in a try/finally
(or try/catch/finally) so setIsRemoving(false) always runs, and keep the
existing setConfirmRemove(false) behavior; update handleRemoveClick to
setIsRemoving(true) before the try, await onRemove inside the try, and move
setIsRemoving(false) into the finally block so failures won't leave the UI
stuck.
In `@console/src/ui/viewer/views/Teams/TeamsPushPanel.tsx`:
- Around line 70-95: The handlePush function can leave the UI stuck if pushAsset
throws; wrap the per-asset loop in a try/catch and ensure setIsPushing(false)
runs in a finally block. Specifically, in handlePush use try { ... for each
asset await pushAsset(...) ... } catch (err) { record the error into
localResults for the failing asset (set success: false and error: String(err))
and update setPushResults } finally { setIsPushing(false) } and only call
onPushComplete when allSucceeded remains true after the try/catch; reference
handlePush, pushAsset, setIsPushing, setPushResults, localResults, and
onPushComplete when making the changes.
- Around line 40-48: The effect in TeamsPushPanel that calls discover() doesn't
handle promise rejection, leaving isLoading true on errors; update the useEffect
so the async call to discover() is awaited inside a try/catch/finally (or add a
.catch/.finally chain) to ensure setIsLoading(false) runs on both success and
failure, still guarding with the mounted flag before calling
setAssets/setRepoUrl; ensure any error is swallowed or logged (e.g., via
console.error or an existing logger) and that the cleanup function sets mounted
= false as before.
In `@console/src/ui/viewer/views/Teams/TeamsSetupTab.tsx`:
- Around line 24-35: handleInit currently calls initTeams directly so if
initTeams throws the async error the UI stays stuck with
setIsInitializing(true); wrap the initTeams call in a try/catch/finally inside
handleInit: in try await initTeams(...); in catch setInitError to the caught
error message (or a friendly fallback) and ensure setIsReconfiguring/setRepoUrl
behavior only on success; in finally always call setIsInitializing(false) so the
Initialize button is never left disabled. Reference symbols: handleInit,
initTeams, setIsInitializing, setInitError, setIsReconfiguring, setRepoUrl.
In `@console/tests/hooks/use-teams.test.ts`:
- Around line 166-179: Replace the brittle source-string assertions in the test
with behavioral tests that mock global.fetch and exercise the hook's exported
actions (discover, pushAsset, removeAsset, initTeams, updateAsset) from
src/ui/viewer/hooks/useTeams.ts: for each action, mock fetch responses and
assert the correct endpoint, HTTP method, and request body were used, then
verify the hook's state/mutation calls (e.g., that isInstalling toggles,
mergeAssets is called with returned assets, removeAsset/updateAsset cause
expected state changes, and initTeams triggers the init endpoint and subsequent
refresh); use jest.fn() or a fetch-mock and clear/reset mocks between tests to
ensure isolation and meaningful failure signals.
In `@console/tests/ui/teams-install.test.ts`:
- Around line 4-71: The current tests only assert source text; add a runtime
behavior test in console/tests/ui/teams-install.test.ts that renders the Teams
view (importing the component from src/ui/viewer/views/Teams/index.tsx), mocks
the sync API and/or the internal sync handler (e.g., handleInstall or
TeamsSyncButton click), simulates a user click on the "Sync All" button, and
asserts that the useToast methods (.success, .error, .warning) are invoked for
success, error, and timeout scenarios; use jest.fn() or your existing test
helpers to spy on the toast returned by useToast() and stub network calls to
drive each outcome.
In `@console/tests/ui/teams-view.test.ts`:
- Around line 87-143: Add interaction tests for TeamsAssetTable that mount the
component (instead of only SSR render) and assert the onUpdate and onRemove
callbacks are invoked through user interactions: locate and click the "Update"
button and verify onUpdate was called; locate and click the "Remove" button,
drive the confirmation dialog flow (click confirm) and verify onRemove was
called; and for uninstalled vault assets locate and click "Install" and verify
appropriate install handler or onUpdate is invoked. Use the same props pattern
from the existing tests, pass jest.fn() spies for onUpdate/onRemove, render
TeamsAssetTable (imported from TeamsAssetTable.js) with tier "team" and "solo"
scenarios, simulate clicks with a DOM testing utility (e.g.,
`@testing-library/react` + userEvent), and assert the spies are called and the
remove-confirmation path triggers onRemove only after confirm.
In `@console/tests/worker/teams-routes.test.ts`:
- Around line 19-37: Add behavior tests for the POST /api/teams/update-asset
route (in addition to the existing registration test) that exercise validation,
command sequencing, and failure paths: instantiate TeamsRoutes, capture the
registered POST handler for "/api/teams/update-asset" (same pattern used to
capture registrations), then call that handler directly with mocked req/res
objects; add separate tests to assert (1) invalid input returns the expected 4xx
validation response, (2) valid input triggers the expected sequence of
operations by mocking and spying the lockfile/installer services (assert call
order and payloads), and (3) when a mocked dependency throws, the handler
returns the correct error status and logs/response; use the same registered
capture mechanism and mock services used elsewhere in this test file to keep
tests focused and isolated.
In `@docs/site/src/pages/docs/TeamsSection.tsx`:
- Around line 10-16: The docs still reference Vault/Roles terminology and the
removed commands; update all occurrences of "Vault", "vault settings", "Vault
Setup" and wording like "manage roles" to the new Teams terminology (e.g.,
"Team" or "Teams") and replace deprecated CLI commands such as `sx vault show`
with the current Teams equivalents (e.g., `sx teams show`), and remove any
instructions about role management flow; update the strings in TeamsSection.tsx
(e.g., the "Vault Setup" and "Push & Pull" entries) and the other affected
ranges (around lines 85-89 and 176-177) to use the new Team/Teams naming and
capabilities.
---
Minor comments:
In `@console/src/ui/viewer/components/CommandPalette.tsx`:
- Around line 124-132: Arrow-key handlers in CommandPalette.tsx compute new
selected index using modulo which yields NaN when filteredCommands.length === 0;
add a guard in the ArrowDown and ArrowUp cases (or before invoking
setSelectedIndex) to no-op when filteredCommands.length === 0 so
setSelectedIndex is not called with invalid math, keeping the current selection
unchanged (or explicitly set to a safe default like -1/0 if that state is
handled elsewhere).
In `@console/src/ui/viewer/views/Dashboard/TeamsStatus.tsx`:
- Around line 68-71: The availability check in TeamsStatus.tsx builds
installedNames from assets using name-only and computes availableCount by
filtering catalog by name, which breaks when assets share names across types;
update the logic to use a composite key (e.g., `${type}::${name}`) for both
installedNames and the catalog filter so matching is done on type+name (ensure
you reference the installedNames set and availableCount calculation), and handle
possible missing/undefined type values consistently when building the composite
keys.
In `@console/src/ui/viewer/views/Teams/index.tsx`:
- Around line 41-42: The Team gating logic is using license which can be null
while the license is still loading; update the code that calls useLicense() to
also read isLoading and defer rendering any TeamGate checks until loading
settles (for example, return a loader or skip rendering TeamGate when isLoading
is true), and then pass the resolved license/tier (or undefined) into TeamGate
instead of null so eligible users don't briefly see locked UI; apply the same
change to all TeamGate usages referenced (the blocks around where useLicense()
is read and TeamGate is rendered).
In `@console/src/ui/viewer/views/Teams/TeamsSetupTab.tsx`:
- Around line 66-71: In TeamsSetupTab (the placeholder ternary using repoType),
replace legacy "vault" wording in the user-facing placeholders: change
"git@github.com:org/team-vault.git" to a non-vault name (e.g.,
"git@github.com:org/team-repo.git") and change "/path/to/vault" to a matching
local path (e.g., "/path/to/team-repo" or "/path/to/teams") so the placeholders
reflect the Teams rename and remain consistent with the UI copy.
In `@console/src/ui/viewer/views/Teams/TeamsSummaryCards.tsx`:
- Around line 49-54: The stat description in TeamsSummaryCards.tsx (the elements
with classNames stat-title, stat-value, stat-desc that display commands and
other via variables commands and other) can become misleading when other > 0;
update the rendering logic for the stat-desc so it conditionally matches the
displayed metric (e.g., when other > 0 show a description like "Slash commands
and other" or similar, otherwise keep "Slash commands") so the description
aligns with the title/value.
In `@console/tests/ui/teams-view.test.ts`:
- Line 116: Rename the test title string in teams-view.test.ts from "renders
install button for uninstalled vault assets" to "renders install button for
uninstalled team assets" so the test name matches the Teams terminology; locate
the it(...) call in the file (the test case starting with it("renders install
button for uninstalled vault assets", ...) and update only the descriptive
string, leaving the test body and assertions unchanged.
In `@README.md`:
- Line 315: The README still uses legacy "Vault" terminology and routes (e.g.,
occurrences of "/vault", "Vault" view, and the legacy file reference
`team-vault.md`) which should be migrated to the new canonical "teams" naming;
search for and replace all references of "/vault" with "/teams", rename mentions
of "Vault" view to "Teams" view (preserving context/capitalization), and update
or remove `team-vault.md` references to the new `teams` documentation or
filename so the docs consistently reflect the product naming.
---
Nitpick comments:
In `@console/src/ui/viewer/views/Teams/index.tsx`:
- Around line 12-19: The formatRepoUrl function is duplicated; extract it into a
shared helper (e.g., export function formatRepoUrl(url: string): string in a
new/util file) and replace the local definition in Teams view(s) with an import
of that helper; update the callers in Teams/index.tsx (and any other Teams view
files using formatRepoUrl) to import the new shared function and remove the
local implementation so URL formatting is centralized.
In `@console/tests/hooks/useSettings.test.ts`:
- Around line 47-53: The test currently checks only presence of six expected
commands and absence of 'vault', so update the assertion to verify the exact set
of keys in DEFAULT_SETTINGS.commands to prevent accidental additions; in the
test for DEFAULT_SETTINGS (in useSettings.test.ts) import DEFAULT_SETTINGS and
replace the per-key checks with a single assertion that
Object.keys(DEFAULT_SETTINGS.commands) (sorted) strictly equals the expected
array (sorted) — referencing DEFAULT_SETTINGS and its commands property to
locate the code to change.
In `@console/tests/ui/teams-navigation.test.ts`:
- Around line 11-49: Replace brittle fs string-search assertions with direct
module imports and assertions against the exported route/nav/command structures:
import the routes export from the module that defines App.tsx routes (e.g., the
exported routes array or a getRoutes function) and assert there is an entry with
component/name "TeamsView" and path "/teams"; import the nav items export from
SidebarNav (e.g., navItems or sidebarConfig) and assert an item with label
"Teams" has icon "users" and href or to "/teams"; import the commands export
from CommandPalette (e.g., commands or paletteItems) and assert a command
labeled "Go to Teams" exists with keybinding "G V" and a navigate handler or
route pointing to "/teams". Ensure tests use the exported symbols (routes,
navItems, commands) rather than reading file text so wiring is verified at
runtime.
In `@console/tests/worker/teams-routes.test.ts`:
- Around line 168-190: The test calls TeamsRoutes.setupRoutes then invokes the
discover handler and uses a sleep to wait; instead, mock external FS/Git
interactions used by TeamsRoutes (e.g., any fs, git, or repo helpers it depends
on) using jest mocks/spies and call the route handler as an async function so
you can await it directly; locate the route via TeamsRoutes.setupRoutes and the
discoverHandler reference, provide a fakeReq/fakeRes that returns a Promise from
json/status (or make the handler return a Promise) and then await
discoverHandler(fakeReq, fakeRes) instead of using setTimeout to make the test
deterministic and avoid touching the real filesystem or git.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
docs/site/api/events/ingest.tsis excluded by!docs/site/api/**docs/site/api/portal/session.tsis excluded by!docs/site/api/**docs/site/api/trial/heartbeat.tsis excluded by!docs/site/api/**docs/site/api/trial/start.tsis excluded by!docs/site/api/**pilot/rules/team-vault.mdis excluded by!pilot/**pilot/scripts/worker-service.cjsis excluded by!pilot/**pilot/settings.jsonis excluded by!pilot/**pilot/ui/viewer-bundle.jsis excluded by!pilot/**pilot/ui/viewer.cssis excluded by!pilot/**
📒 Files selected for processing (38)
README.mdconsole/src/services/worker-service.tsconsole/src/services/worker/http/routes/SettingsRoutes.tsconsole/src/services/worker/http/routes/TeamsRoutes.tsconsole/src/services/worker/http/routes/VaultRoutes.tsconsole/src/ui/viewer/App.tsxconsole/src/ui/viewer/components/CommandPalette.tsxconsole/src/ui/viewer/components/TeamGate.tsxconsole/src/ui/viewer/constants/shortcuts.tsconsole/src/ui/viewer/hooks/useSettings.tsconsole/src/ui/viewer/hooks/useStats.tsconsole/src/ui/viewer/hooks/useTeams.tsconsole/src/ui/viewer/layouts/Sidebar/SidebarNav.tsxconsole/src/ui/viewer/views/Dashboard/TeamsStatus.tsxconsole/src/ui/viewer/views/Dashboard/index.tsxconsole/src/ui/viewer/views/Teams/TeamsAssetDetail.tsxconsole/src/ui/viewer/views/Teams/TeamsAssetTable.tsxconsole/src/ui/viewer/views/Teams/TeamsPushPanel.tsxconsole/src/ui/viewer/views/Teams/TeamsSetupTab.tsxconsole/src/ui/viewer/views/Teams/TeamsSummaryCards.tsxconsole/src/ui/viewer/views/Teams/index.tsxconsole/src/ui/viewer/views/Vault/index.tsxconsole/src/ui/viewer/views/index.tsconsole/tests/hooks/use-teams.test.tsconsole/tests/hooks/useSettings.test.tsconsole/tests/settings-routes.test.tsconsole/tests/ui/SidebarNav.test.tsconsole/tests/ui/search-removal.test.tsconsole/tests/ui/teams-install.test.tsconsole/tests/ui/teams-navigation.test.tsconsole/tests/ui/teams-view.test.tsconsole/tests/ui/vault-install.test.tsconsole/tests/ui/vault-navigation.test.tsconsole/tests/ui/vault-view.test.tsconsole/tests/ui/views-index.test.tsconsole/tests/worker/teams-routes.test.tsdocs/site/src/pages/docs/TeamsSection.tsxinstaller/steps/finalize.py
💤 Files with no reviewable changes (9)
- console/src/ui/viewer/hooks/useSettings.ts
- console/tests/ui/vault-install.test.ts
- console/tests/settings-routes.test.ts
- console/src/services/worker/http/routes/SettingsRoutes.ts
- console/tests/ui/vault-view.test.ts
- console/tests/ui/vault-navigation.test.ts
- console/src/services/worker/http/routes/VaultRoutes.ts
- console/src/ui/viewer/views/Vault/index.tsx
- installer/steps/finalize.py
Comment on lines
+282
to
+290
| const projectRoot = process.env.CLAUDE_PROJECT_ROOT || process.cwd(); | ||
| const resolvedSource = path.resolve(projectRoot, source); | ||
| if ( | ||
| resolvedSource !== projectRoot && | ||
| !resolvedSource.startsWith(projectRoot + path.sep) | ||
| ) { | ||
| res.status(400).json({ error: "Source path must be within project" }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Source-path validation is vulnerable to symlink escape.
Prefix checks on path.resolve(...) do not prevent paths that traverse outside the project via symlinks.
🔒 Suggested hardening
+import { readdirSync, existsSync, realpathSync } from "fs";
...
const projectRoot = process.env.CLAUDE_PROJECT_ROOT || process.cwd();
const resolvedSource = path.resolve(projectRoot, source);
+ let realProjectRoot: string;
+ let realSource: string;
+ try {
+ realProjectRoot = realpathSync(projectRoot);
+ realSource = realpathSync(resolvedSource);
+ } catch {
+ res.status(400).json({ error: "Source path is invalid or does not exist" });
+ return;
+ }
if (
- resolvedSource !== projectRoot &&
- !resolvedSource.startsWith(projectRoot + path.sep)
+ realSource !== realProjectRoot &&
+ !realSource.startsWith(realProjectRoot + path.sep)
) {
res.status(400).json({ error: "Source path must be within project" });
return;
}📝 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
| const projectRoot = process.env.CLAUDE_PROJECT_ROOT || process.cwd(); | |
| const resolvedSource = path.resolve(projectRoot, source); | |
| if ( | |
| resolvedSource !== projectRoot && | |
| !resolvedSource.startsWith(projectRoot + path.sep) | |
| ) { | |
| res.status(400).json({ error: "Source path must be within project" }); | |
| return; | |
| } | |
| const projectRoot = process.env.CLAUDE_PROJECT_ROOT || process.cwd(); | |
| const resolvedSource = path.resolve(projectRoot, source); | |
| let realProjectRoot: string; | |
| let realSource: string; | |
| try { | |
| realProjectRoot = realpathSync(projectRoot); | |
| realSource = realpathSync(resolvedSource); | |
| } catch { | |
| res.status(400).json({ error: "Source path is invalid or does not exist" }); | |
| return; | |
| } | |
| if ( | |
| realSource !== realProjectRoot && | |
| !realSource.startsWith(realProjectRoot + path.sep) | |
| ) { | |
| res.status(400).json({ error: "Source path must be within project" }); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@console/src/services/worker/http/routes/TeamsRoutes.ts` around lines 282 -
290, The current prefix check on resolvedSource can be bypassed via symlinks;
change validation to compare real filesystem paths: call fs.promises.realpath
(or fs.realpathSync) for projectRoot and resolvedSource (referencing projectRoot
and resolvedSource variables and path.resolve usage) and verify the
resolvedSourceRealpath equals projectRootRealpath or
startsWith(projectRootRealpath + path.sep) before accepting; return the same 400
error on failure. Ensure to handle realpath errors (treat as invalid) and
preserve use of process.env.CLAUDE_PROJECT_ROOT when computing
projectRootRealpath.
Comment on lines
+320
to
+324
| } catch (error) { | ||
| const message = (error as Error).message || "Push failed"; | ||
| logger.error("HTTP", "Teams push failed", { name }, error as Error); | ||
| res.json({ success: false, error: message }); | ||
| } |
There was a problem hiding this comment.
Failures are returned as HTTP 200 on mutating endpoints.
Returning 200 for operational failures makes monitoring/retries and client handling ambiguous. Use non-2xx status codes for failed mutations.
🔧 Suggested response semantics
- res.json({ success: false, error: message });
+ res.status(502).json({ success: false, error: message });Apply the same pattern in handleRemove, handleInit, and handleUpdateAsset.
Also applies to: 355-358, 394-397, 503-512
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@console/src/services/worker/http/routes/TeamsRoutes.ts` around lines 320 -
324, The catch block in the Teams push handler currently returns a 200 OK with
an error payload; update the error handling to send an appropriate non-2xx HTTP
status (e.g., res.status(500)) before returning the JSON error, and include the
same message and logger.error usage (see the catch that defines const message =
(error as Error).message and calls logger.error("HTTP", "Teams push failed", {
name }, error as Error)); apply the identical change to the catch blocks in
handleRemove, handleInit, and handleUpdateAsset so failures return a non-2xx
status with the same error JSON and logging behavior.
Comment on lines
+457
to
+468
| private handleUpdateAsset = this.wrapHandler( | ||
| async (req: Request, res: Response): Promise<void> => { | ||
| const { name, currentVersion, scope, scopeUrl } = req.body; | ||
| if (!name || !NAME_REGEX.test(name)) { | ||
| res.status(400).json({ error: "Invalid asset name" }); | ||
| return; | ||
| } | ||
| const sxPath = this.resolveSxBinary(); | ||
| if (!sxPath) { | ||
| res.status(500).json({ error: "sx CLI not found" }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
update-asset is not synchronized with ongoing installs.
This endpoint performs lockfile mutation plus install but does not check/set _isInstalling, so concurrent install/update requests can race and corrupt state.
🧵 Suggested synchronization fix
private handleUpdateAsset = this.wrapHandler(
async (req: Request, res: Response): Promise<void> => {
+ if (this._isInstalling) {
+ res.status(409).json({ error: "Installation already in progress" });
+ return;
+ }
+ this._isInstalling = true;
+ this.statusCache = null;
+
const { name, currentVersion, scope, scopeUrl } = req.body;
if (!name || !NAME_REGEX.test(name)) {
res.status(400).json({ error: "Invalid asset name" });
+ this._isInstalling = false;
return;
}
...
try {
// Remove old version from lock file
...
await this.runSxCommand(
[sxPath, "install", "--repair", "--target", projectRoot],
INSTALL_TIMEOUT_MS,
);
this.statusCache = null;
this.detailCache.clear();
res.json({ success: true, error: null });
} catch (error) {
...
res.json({ success: false, error: message });
+ } finally {
+ this._isInstalling = false;
}
},
);Also applies to: 495-499
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@console/src/services/worker/http/routes/TeamsRoutes.ts` around lines 457 -
468, The update-asset handler (handleUpdateAsset) mutates the lockfile and runs
installs without synchronizing with concurrent installs; guard the critical
section by checking and setting the shared flag _isInstalling: at the start of
the handler, if (this._isInstalling) return a 409/conflict response, otherwise
set this._isInstalling = true before performing lockfile mutation and the
install, and ensure you clear it in a finally block so it’s always reset; apply
the same pattern to the other install/update handler referenced around lines
495-499 (the other route handler that mutates the lockfile) so both use the same
check/set/try...finally/unset sequence and avoid race conditions.
Comment on lines
+470
to
+494
| // Remove old version from lock file | ||
| if (currentVersion) { | ||
| await this.runSxCommand( | ||
| [ | ||
| sxPath, | ||
| "remove", | ||
| name, | ||
| "--version", | ||
| String(currentVersion), | ||
| "--yes", | ||
| ], | ||
| REMOVE_TIMEOUT_MS, | ||
| ); | ||
| } | ||
| // Re-add with scope (picks up latest version) | ||
| const addArgs = [sxPath, "add", name, "--yes"]; | ||
| if (scope === "global") { | ||
| addArgs.push("--scope-global"); | ||
| } else if (scopeUrl) { | ||
| addArgs.push("--scope-repo", scopeUrl); | ||
| } else { | ||
| addArgs.push("--scope-global"); | ||
| } | ||
| await this.runSxCommand(addArgs, PUSH_TIMEOUT_MS); | ||
| // Install the updated version |
There was a problem hiding this comment.
Remove-first update flow can leave the asset missing on partial failure.
If remove succeeds and add or install fails, the lockfile is left in a degraded state. Add a compensating rollback or use an update sequence that commits only after success.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@console/src/services/worker/http/routes/TeamsRoutes.ts` around lines 470 -
494, The current remove-then-add sequence can leave the lockfile degraded if
add/install fails; change the flow in the method that calls runSxCommand so you
add the new asset before removing the old (build addArgs and call
runSxCommand(addArgs, PUSH_TIMEOUT_MS) first), and only on successful add call
runSxCommand([...sxPath, "remove", name, "--version", String(currentVersion),
"--yes"], REMOVE_TIMEOUT_MS) to drop the old entry; additionally wrap both
operations in try/catch and on any failure attempt a compensating rollback by
calling runSxCommand([...sxPath, "add", name, "--version",
String(currentVersion), "--yes"], PUSH_TIMEOUT_MS) to restore the previous
version (use currentVersion, addArgs, runSxCommand, REMOVE_TIMEOUT_MS,
PUSH_TIMEOUT_MS to locate code).
Comment on lines
+16
to
+18
| <div className="opacity-30 pointer-events-none blur-[2px] select-none"> | ||
| {children} | ||
| </div> |
There was a problem hiding this comment.
Locked content remains keyboard-focusable behind the overlay.
pointer-events-none only blocks pointer interaction. Underlying focusable controls can still be reached via keyboard/assistive tech.
🔧 Suggested hardening
- <div className="opacity-30 pointer-events-none blur-[2px] select-none">
+ <div
+ inert
+ aria-hidden="true"
+ className="opacity-30 pointer-events-none blur-[2px] select-none"
+ >
{children}
</div>📝 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
| <div className="opacity-30 pointer-events-none blur-[2px] select-none"> | |
| {children} | |
| </div> | |
| <div | |
| inert | |
| aria-hidden="true" | |
| className="opacity-30 pointer-events-none blur-[2px] select-none" | |
| > | |
| {children} | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@console/src/ui/viewer/components/TeamGate.tsx` around lines 16 - 18, The
overlay wrapper currently uses pointer-events-none which doesn't prevent
keyboard focus; update the wrapper div in TeamGate (the element rendering
{children}) to remove keyboard access by adding accessibility attributes: set
aria-hidden="true" and add the inert attribute (and fallback to tabIndex={-1} if
inert not supported) on that div so underlying interactive controls are excluded
from the accessibility tree and cannot receive keyboard focus; ensure the same
element keeps the visual classes (opacity-30 blur-[2px] select-none) while
adding these attributes.
Comment on lines
166
to
179
| it("source contains API endpoint fetch logic", async () => { | ||
| const fs = await import("fs"); | ||
| const source = fs.readFileSync("src/ui/viewer/hooks/useVault.ts", "utf-8"); | ||
| const source = fs.readFileSync("src/ui/viewer/hooks/useTeams.ts", "utf-8"); | ||
|
|
||
| expect(source).toContain("/api/vault/status"); | ||
| expect(source).toContain("/api/vault/detail/"); | ||
| expect(source).toContain("/api/vault/install"); | ||
| expect(source).toContain("/api/teams/status"); | ||
| expect(source).toContain("/api/teams/detail/"); | ||
| expect(source).toContain("/api/teams/install"); | ||
| expect(source).toContain("isInstalling"); | ||
| expect(source).toContain("mergeAssets"); | ||
| expect(source).toContain("/api/teams/remove"); | ||
| expect(source).toContain("removeAsset"); | ||
| expect(source).toContain("/api/teams/init"); | ||
| expect(source).toContain("initTeams"); | ||
| }); |
There was a problem hiding this comment.
Replace source-string assertions with behavioral fetch-mocked tests for Teams mutations.
This block can pass even when runtime behavior regresses (wrong method/body, missing refresh, wrong endpoint usage). For this PR, add behavior tests around discover, pushAsset, removeAsset, initTeams, and updateAsset using mocked fetch.
As per coding guidelines, "/tests/: Review test code briefly. Focus on: Test coverage for the feature being tested; Proper mocking of external dependencies; Clear test names that describe behavior**."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@console/tests/hooks/use-teams.test.ts` around lines 166 - 179, Replace the
brittle source-string assertions in the test with behavioral tests that mock
global.fetch and exercise the hook's exported actions (discover, pushAsset,
removeAsset, initTeams, updateAsset) from src/ui/viewer/hooks/useTeams.ts: for
each action, mock fetch responses and assert the correct endpoint, HTTP method,
and request body were used, then verify the hook's state/mutation calls (e.g.,
that isInstalling toggles, mergeAssets is called with returned assets,
removeAsset/updateAsset cause expected state changes, and initTeams triggers the
init endpoint and subsequent refresh); use jest.fn() or a fetch-mock and
clear/reset mocks between tests to ensure isolation and meaningful failure
signals.
Comment on lines
+4
to
+71
| it('TeamsSyncButton component exists and renders correctly', async () => { | ||
| const fs = await import('fs/promises'); | ||
| const teamsViewContent = await fs.readFile( | ||
| 'src/ui/viewer/views/Teams/index.tsx', | ||
| 'utf-8' | ||
| ); | ||
|
|
||
| expect(teamsViewContent).toContain('function TeamsSyncButton'); | ||
|
|
||
| expect(teamsViewContent).toMatch(/className="btn btn-primary btn-sm"/); | ||
|
|
||
| expect(teamsViewContent).toContain('loading loading-spinner'); | ||
| expect(teamsViewContent).toContain('Syncing...'); | ||
|
|
||
| expect(teamsViewContent).toContain('Sync All'); | ||
| }); | ||
|
|
||
| it('TeamsView uses useToast for notifications', async () => { | ||
| const fs = await import('fs/promises'); | ||
| const teamsViewContent = await fs.readFile( | ||
| 'src/ui/viewer/views/Teams/index.tsx', | ||
| 'utf-8' | ||
| ); | ||
|
|
||
| expect(teamsViewContent).toMatch(/import.*useToast.*from.*ToastContext/); | ||
|
|
||
| expect(teamsViewContent).toMatch(/useToast\(\)/); | ||
| }); | ||
|
|
||
| it('handles successful install with toast notification', async () => { | ||
| const fs = await import('fs/promises'); | ||
| const teamsViewContent = await fs.readFile( | ||
| 'src/ui/viewer/views/Teams/index.tsx', | ||
| 'utf-8' | ||
| ); | ||
|
|
||
| expect(teamsViewContent).toMatch(/\.success\(['"].*[Tt]eams.*sync/i); | ||
| }); | ||
|
|
||
| it('handles install error with toast notification', async () => { | ||
| const fs = await import('fs/promises'); | ||
| const teamsViewContent = await fs.readFile( | ||
| 'src/ui/viewer/views/Teams/index.tsx', | ||
| 'utf-8' | ||
| ); | ||
|
|
||
| expect(teamsViewContent).toMatch(/\.error\(/); | ||
| }); | ||
|
|
||
| it('handles install timeout with warning toast', async () => { | ||
| const fs = await import('fs/promises'); | ||
| const teamsViewContent = await fs.readFile( | ||
| 'src/ui/viewer/views/Teams/index.tsx', | ||
| 'utf-8' | ||
| ); | ||
|
|
||
| expect(teamsViewContent).toMatch(/\.warning\(/); | ||
| }); | ||
|
|
||
| it('handleInstall wrapper function exists', async () => { | ||
| const fs = await import('fs/promises'); | ||
| const teamsViewContent = await fs.readFile( | ||
| 'src/ui/viewer/views/Teams/index.tsx', | ||
| 'utf-8' | ||
| ); | ||
|
|
||
| expect(teamsViewContent).toMatch(/handle(Install|Sync)/); | ||
| }); |
There was a problem hiding this comment.
These tests validate source text, not install behavior.
All assertions are string/regex checks on file content, so they can still pass if sync handlers or toast wiring break at runtime. Add at least one behavior-level test that renders Teams view, triggers sync, and asserts success/error/timeout toast calls.
As per coding guidelines, "Review test code briefly. Focus on: Test coverage for the feature being tested".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@console/tests/ui/teams-install.test.ts` around lines 4 - 71, The current
tests only assert source text; add a runtime behavior test in
console/tests/ui/teams-install.test.ts that renders the Teams view (importing
the component from src/ui/viewer/views/Teams/index.tsx), mocks the sync API
and/or the internal sync handler (e.g., handleInstall or TeamsSyncButton click),
simulates a user click on the "Sync All" button, and asserts that the useToast
methods (.success, .error, .warning) are invoked for success, error, and timeout
scenarios; use jest.fn() or your existing test helpers to spy on the toast
returned by useToast() and stub network calls to drive each outcome.
Comment on lines
+87
to
+143
| it("renders action buttons for installed assets with update", async () => { | ||
| const { TeamsAssetTable } = await import("../../src/ui/viewer/views/Teams/TeamsAssetTable.js"); | ||
|
|
||
| const assets = [ | ||
| { name: "my-skill", type: "skill", latestVersion: "3", versionsCount: 3, installedVersion: "v2", installed: true, hasUpdate: true, scope: "Global", clients: [] }, | ||
| ]; | ||
|
|
||
| const html = renderToString( | ||
| React.createElement(TeamsAssetTable, { | ||
| assets, | ||
| searchQuery: "", | ||
| activeTab: "all", | ||
| onTabChange: () => {}, | ||
| onSearchChange: () => {}, | ||
| expandedAsset: null, | ||
| onAssetClick: () => {}, | ||
| fetchDetail: () => Promise.resolve(), | ||
| detailCache: new Map(), | ||
| loadingDetails: new Set<string>(), | ||
| tier: "team", | ||
| onUpdate: () => {}, | ||
| onRemove: () => Promise.resolve(), | ||
| }), | ||
| ); | ||
|
|
||
| expect(html).toContain("Update"); | ||
| expect(html).toContain("Remove"); | ||
| }); | ||
|
|
||
| it("renders install button for uninstalled vault assets", async () => { | ||
| const { TeamsAssetTable } = await import("../../src/ui/viewer/views/Teams/TeamsAssetTable.js"); | ||
|
|
||
| const assets = [ | ||
| { name: "my-rule", type: "rule", latestVersion: "1", versionsCount: 1, installedVersion: null, installed: false, hasUpdate: false, scope: null, clients: [] }, | ||
| ]; | ||
|
|
||
| const html = renderToString( | ||
| React.createElement(TeamsAssetTable, { | ||
| assets, | ||
| searchQuery: "", | ||
| activeTab: "all", | ||
| onTabChange: () => {}, | ||
| onSearchChange: () => {}, | ||
| expandedAsset: null, | ||
| onAssetClick: () => {}, | ||
| fetchDetail: () => Promise.resolve(), | ||
| detailCache: new Map(), | ||
| loadingDetails: new Set<string>(), | ||
| tier: "solo", | ||
| onUpdate: () => {}, | ||
| onRemove: () => Promise.resolve(), | ||
| }), | ||
| ); | ||
|
|
||
| expect(html).toContain("Install"); | ||
| expect(html).not.toContain("Remove"); | ||
| }); |
There was a problem hiding this comment.
CRUD action coverage is incomplete for TeamsAssetTable.
These tests only check button labels in SSR output. Please add interaction-level tests that verify onUpdate/onRemove are invoked correctly, including the remove confirmation path.
As per coding guidelines, "Review test code briefly. Focus on: Test coverage for the feature being tested".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@console/tests/ui/teams-view.test.ts` around lines 87 - 143, Add interaction
tests for TeamsAssetTable that mount the component (instead of only SSR render)
and assert the onUpdate and onRemove callbacks are invoked through user
interactions: locate and click the "Update" button and verify onUpdate was
called; locate and click the "Remove" button, drive the confirmation dialog flow
(click confirm) and verify onRemove was called; and for uninstalled vault assets
locate and click "Install" and verify appropriate install handler or onUpdate is
invoked. Use the same props pattern from the existing tests, pass jest.fn()
spies for onUpdate/onRemove, render TeamsAssetTable (imported from
TeamsAssetTable.js) with tier "team" and "solo" scenarios, simulate clicks with
a DOM testing utility (e.g., `@testing-library/react` + userEvent), and assert the
spies are called and the remove-confirmation path triggers onRemove only after
confirm.
Comment on lines
+19
to
+37
| it("registers all 8 routes", () => { | ||
| const routes = new TeamsRoutes(); | ||
| const registered: { method: string; path: string }[] = []; | ||
| const fakeApp = { | ||
| get: (path: string, _handler: any) => registered.push({ method: "GET", path }), | ||
| post: (path: string, _handler: any) => registered.push({ method: "POST", path }), | ||
| }; | ||
| routes.setupRoutes(fakeApp as any); | ||
|
|
||
| expect(registered).toContainEqual({ method: "GET", path: "/api/vault/status" }); | ||
| expect(registered).toContainEqual({ method: "POST", path: "/api/vault/install" }); | ||
| expect(registered).toContainEqual({ method: "GET", path: "/api/vault/detail/:name" }); | ||
| expect(registered).toContainEqual({ method: "GET", path: "/api/teams/status" }); | ||
| expect(registered).toContainEqual({ method: "POST", path: "/api/teams/install" }); | ||
| expect(registered).toContainEqual({ method: "GET", path: "/api/teams/detail/:name" }); | ||
| expect(registered).toContainEqual({ method: "POST", path: "/api/teams/push" }); | ||
| expect(registered).toContainEqual({ method: "POST", path: "/api/teams/remove" }); | ||
| expect(registered).toContainEqual({ method: "POST", path: "/api/teams/init" }); | ||
| expect(registered).toContainEqual({ method: "GET", path: "/api/teams/discover" }); | ||
| expect(registered).toContainEqual({ method: "POST", path: "/api/teams/update-asset" }); | ||
| expect(registered.length).toBe(8); | ||
| }); |
There was a problem hiding this comment.
/api/teams/update-asset is only registration-tested, not behavior-tested.
Given it mutates lockfile/install state, add endpoint behavior tests (validation, command sequencing, and failure-path expectations).
As per coding guidelines, "**/tests/**: Review test code briefly. Focus on: Test coverage for the feature being tested".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@console/tests/worker/teams-routes.test.ts` around lines 19 - 37, Add behavior
tests for the POST /api/teams/update-asset route (in addition to the existing
registration test) that exercise validation, command sequencing, and failure
paths: instantiate TeamsRoutes, capture the registered POST handler for
"/api/teams/update-asset" (same pattern used to capture registrations), then
call that handler directly with mocked req/res objects; add separate tests to
assert (1) invalid input returns the expected 4xx validation response, (2) valid
input triggers the expected sequence of operations by mocking and spying the
lockfile/installer services (assert call order and payloads), and (3) when a
mocked dependency throws, the handler returns the correct error status and
logs/response; use the same registered capture mechanism and mock services used
elsewhere in this test file to keep tests focused and isolated.
Comment on lines
+10
to
+16
| title: "Push & Pull", | ||
| desc: "Share custom rules, skills, and commands with teammates via Git vault", | ||
| }, | ||
| { | ||
| title: "Vault Setup", | ||
| desc: "Configure your team vault (Git repo, local path, or Skills.new) from the UI", | ||
| }, |
There was a problem hiding this comment.
Docs still describe removed Vault/Roles flows.
This content still instructs users about “vault settings”, “manage roles”, and sx vault show, but this PR removes Roles and migrates Vault naming to Teams. Please update wording/commands to the current Teams terminology and capabilities.
Also applies to: 85-89, 176-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/site/src/pages/docs/TeamsSection.tsx` around lines 10 - 16, The docs
still reference Vault/Roles terminology and the removed commands; update all
occurrences of "Vault", "vault settings", "Vault Setup" and wording like "manage
roles" to the new Teams terminology (e.g., "Team" or "Teams") and replace
deprecated CLI commands such as `sx vault show` with the current Teams
equivalents (e.g., `sx teams show`), and remove any instructions about role
management flow; update the strings in TeamsSection.tsx (e.g., the "Vault Setup"
and "Push & Pull" entries) and the other affected ranges (around lines 85-89 and
176-177) to use the new Team/Teams naming and capabilities.
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.
Summary
POST /api/teams/update-assetendpoint that properly updates lock file versions viasx remove+sx add(instead of justsx install --repairwhich only reinstalls pinned versions)Vault*types toTeams*,vaultUrl→repoUrlacross the full stack, cleaned up references in settings.json, installer, rules, README/vault→/teams, sidebar, command palette, keyboard shortcutsTest plan
Summary by CodeRabbit
Release Notes
New Features
Refactor
Documentation