Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis pull request adds workspace environment setup infrastructure with a new configuration script that manages per-branch port allocation, exports workspace variables, and initializes workspace windows in the development environment. The setup ensures Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.config/wt/env.sh (1)
32-33: Make PATH prepend idempotent.Lines 32–33 prepend
WK_BIN_DIRevery time the script is sourced, which can bloat PATH across window/session reloads.Suggested fix
export WK_BIN_DIR="$REPO_ROOT/.config/wt/bin" -export PATH="$WK_BIN_DIR:$PATH" +case ":$PATH:" in + *":$WK_BIN_DIR:"*) ;; + *) export PATH="$WK_BIN_DIR:$PATH" ;; +esac🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.config/wt/env.sh around lines 32 - 33, Prepend to PATH only if WK_BIN_DIR is not already present: ensure WK_BIN_DIR is defined, check PATH for an exact entry (use colon-wrapped comparison to avoid partial matches) and only then set PATH="$WK_BIN_DIR:$PATH" and export; this makes the export of WK_BIN_DIR and PATH idempotent across repeated sourcing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.config/wk.yml:
- Line 7: The term value currently references SHELL directly which can be unset;
update the term string to use shell parameter expansion to provide a safe
fallback (e.g., fall back to /bin/sh or /bin/bash) when SHELL is undefined so
the echo+exec sequence won't fail; specifically modify the "term" entry in the
YAML to replace plain $SHELL with a fallback expansion (use the existing term
key as the location to change).
In @.config/wt/env.sh:
- Line 8: The BRANCH assignment uses git in the caller's CWD so it can pick the
wrong branch when the script is sourced outside the repo; change it to resolve
git operations from the repository root instead. Update the BRANCH logic (the
BRANCH variable assignment) to first get the repo root (via git rev-parse
--show-toplevel 2>/dev/null) and then run git --show-current with -C pointing at
that root, falling back to "main" if those git calls fail.
---
Nitpick comments:
In @.config/wt/env.sh:
- Around line 32-33: Prepend to PATH only if WK_BIN_DIR is not already present:
ensure WK_BIN_DIR is defined, check PATH for an exact entry (use colon-wrapped
comparison to avoid partial matches) and only then set PATH="$WK_BIN_DIR:$PATH"
and export; this makes the export of WK_BIN_DIR and PATH idempotent across
repeated sourcing.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6fbb3c80-a81d-499b-9599-131f27b1fe43
📒 Files selected for processing (3)
.config/wk.yml.config/wt/env.sh.worktreeinclude
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a workspace environment configuration by adding a dynamic environment setup script and defining workspace window commands. The changes include a new shell script to manage branch-specific ports and environment variables, and updates to the workspace configuration file. Feedback was provided to improve the robustness of the port hashing function against missing dependencies and to prevent duplicate entries in the PATH variable when the script is sourced multiple times.
📊 Coverage ReportOverall Coverage: 45% Diff: origin/main...HEADNo lines with coverage information in this diff.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/server/kiln_server/utils/agent_checks/policy_lookup.py (1)
32-40: Remove the deprecated_load()method to eliminate dead code.The
_load()method is never called internally within the class and has no external callers. Since_loadis private by convention, removing it outright is cleaner than maintaining a deprecation shim for unused code. The public API is alreadypreload()andget_policy(), both of which work correctly without it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/server/kiln_server/utils/agent_checks/policy_lookup.py` around lines 32 - 40, The private deprecated method _load() is dead code; remove the entire _load method (the warnings.warn call and self.preload() invocation) from policy_lookup.py and any associated docstring or comments so the class exposes only the public preload() and get_policy() API; after removal, run linters/tests and delete any now-unused imports (e.g., warnings) if they become unused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/server/kiln_server/utils/agent_checks/policy_lookup.py`:
- Around line 32-40: The private deprecated method _load() is dead code; remove
the entire _load method (the warnings.warn call and self.preload() invocation)
from policy_lookup.py and any associated docstring or comments so the class
exposes only the public preload() and get_policy() API; after removal, run
linters/tests and delete any now-unused imports (e.g., warnings) if they become
unused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a87c3db2-5de3-4f5d-8939-0bdcc8ef8926
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.agents/claude/setup.sh.config/wk.yml.config/wt/.worktreeinclude.config/wt/env.sh.config/wt/layout.kdl.github/workflows/check_api_bindings.yml.github/workflows/test_count.yml.github/workflows/web_format_lint_build.yml.gitignoreCONTRIBUTING.mdMakefileapp/desktop/studio_server/chat/routes.pyapp/desktop/studio_server/chat/test_routes.pyapp/web_ui/.nvmrcapp/web_ui/src/lib/stores/local_storage_store.tsapp/web_ui/src/routes/(app)/dataset/[project_id]/[task_id]/[run_id]/run/+page.svelteapp/web_ui/src/routes/(app)/run/run.sveltelibs/server/kiln_server/utils/agent_checks/policy_lookup.pylibs/server/kiln_server/utils/agent_checks/test_policy_lookup.py
✅ Files skipped from review due to trivial changes (7)
- .gitignore
- app/web_ui/.nvmrc
- .agents/claude/setup.sh
- .github/workflows/check_api_bindings.yml
- .github/workflows/test_count.yml
- .config/wk.yml
- .config/wt/env.sh
🚧 Files skipped from review as they are similar to previous changes (8)
- CONTRIBUTING.md
- .github/workflows/web_format_lint_build.yml
- app/desktop/studio_server/chat/routes.py
- .config/wt/layout.kdl
- libs/server/kiln_server/utils/agent_checks/test_policy_lookup.py
- app/web_ui/src/lib/stores/local_storage_store.ts
- app/desktop/studio_server/chat/test_routes.py
- Makefile
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/web_ui/src/lib/ui/kiln_settings_row.svelte (2)
57-90: Consider extracting the inline SVGs to icon components for consistency.The rest of the settings page uses dedicated icon components from
$lib/ui/icons/*. Inlining these two glyphs here diverges from that pattern and makes future swaps (e.g., new external-link icon) harder. Low priority since the component is self-contained.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/lib/ui/kiln_settings_row.svelte` around lines 57 - 90, The inline SVGs in kiln_settings_row.svelte (the conditional branch for {`#if` href && is_external} and the {:else} chevron) should be replaced with icon components for consistency; create or import icon components (e.g., ExternalLinkIcon and ChevronRightIcon) from $lib/ui/icons, pass through the existing class="w-full h-full" and aria-hidden="true" props (or class/style slot) so sizing/visibility stays the same, and then remove the inline SVG blocks and their duplicated attributes, keeping the {`#if` href && is_external} conditional and span wrapper intact and ensuring the icons render based on the same is_external/href logic.
41-48: Nit: use DaisyUI/Tailwind token for the warning dot color.The inline hex
#F4B544bypasses the theme. A DaisyUI token (e.g.,bg-warning) keeps styling consistent and theme-aware:🎨 Suggested tweak
- <span - class="inline-block w-2 h-2 rounded-full flex-shrink-0" - style="background-color: `#F4B544`;" - role="img" - aria-label="Warning" - ></span> + <span + class="inline-block w-2 h-2 rounded-full flex-shrink-0 bg-warning" + role="img" + aria-label="Warning" + ></span>As per coding guidelines: "Use DaisyUI for UI components in the web frontend".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/lib/ui/kiln_settings_row.svelte` around lines 41 - 48, The warning dot uses an inline style color (`#F4B544`) which bypasses theme tokens; replace the inline style on the <span> rendered when status === "warn" with a DaisyUI/Tailwind background token (e.g., add the class bg-warning) so it respects theming, keeping the existing classes (inline-block w-2 h-2 rounded-full flex-shrink-0) and the role/aria-label attributes intact; update the span in the conditional block that checks status === "warn" to use the token-based class instead of style.app/web_ui/src/routes/(app)/settings/+page.svelte (1)
31-44: Row type allows bothhrefandon_click, but only one is actually honored.
KilnSettingsRowwireson:clickonly whenhrefis falsy, so a row declaring both would silently lose its click handler. Current usages are mutually exclusive, so no bug today — consider a discriminated union if you want the type system to enforce this:♻️ Optional stricter typing
- type SettingsRow = { - label: string - icon: ComponentType - detail?: string - href?: string - on_click?: () => void - is_external?: boolean - status?: "warn" - } + type SettingsRowBase = { + label: string + icon: ComponentType + detail?: string + status?: "warn" + } + type SettingsRow = + | (SettingsRowBase & { href: string; is_external?: boolean; on_click?: never }) + | (SettingsRowBase & { on_click: () => void; href?: never; is_external?: never })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/settings/+page.svelte around lines 31 - 44, The SettingsRow type currently allows both href and on_click which is misleading because KilnSettingsRow only wires on:click when href is falsy; change SettingsRow to a discriminated union so callers cannot provide both: create two variants (e.g., { href: string; is_external?: boolean; icon: ComponentType; label: string; detail?: string; status?: "warn" } and { on_click: () => void; icon: ComponentType; label: string; detail?: string; status?: "warn" }) and update any code that builds rows to conform to the appropriate variant; keep the KilnSettingsRow wiring logic unchanged but adjust its prop type to accept the union so the type system enforces mutual exclusivity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/web_ui/src/lib/ui/kiln_settings_row.svelte`:
- Around line 57-90: The inline SVGs in kiln_settings_row.svelte (the
conditional branch for {`#if` href && is_external} and the {:else} chevron) should
be replaced with icon components for consistency; create or import icon
components (e.g., ExternalLinkIcon and ChevronRightIcon) from $lib/ui/icons,
pass through the existing class="w-full h-full" and aria-hidden="true" props (or
class/style slot) so sizing/visibility stays the same, and then remove the
inline SVG blocks and their duplicated attributes, keeping the {`#if` href &&
is_external} conditional and span wrapper intact and ensuring the icons render
based on the same is_external/href logic.
- Around line 41-48: The warning dot uses an inline style color (`#F4B544`) which
bypasses theme tokens; replace the inline style on the <span> rendered when
status === "warn" with a DaisyUI/Tailwind background token (e.g., add the class
bg-warning) so it respects theming, keeping the existing classes (inline-block
w-2 h-2 rounded-full flex-shrink-0) and the role/aria-label attributes intact;
update the span in the conditional block that checks status === "warn" to use
the token-based class instead of style.
In `@app/web_ui/src/routes/`(app)/settings/+page.svelte:
- Around line 31-44: The SettingsRow type currently allows both href and
on_click which is misleading because KilnSettingsRow only wires on:click when
href is falsy; change SettingsRow to a discriminated union so callers cannot
provide both: create two variants (e.g., { href: string; is_external?: boolean;
icon: ComponentType; label: string; detail?: string; status?: "warn" } and {
on_click: () => void; icon: ComponentType; label: string; detail?: string;
status?: "warn" }) and update any code that builds rows to conform to the
appropriate variant; keep the KilnSettingsRow wiring logic unchanged but adjust
its prop type to accept the union so the type system enforces mutual
exclusivity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 65b9d353-835c-4275-b33a-8546dd766472
📒 Files selected for processing (12)
app/web_ui/src/lib/ui/icons/book_icon.svelteapp/web_ui/src/lib/ui/icons/cube_icon.svelteapp/web_ui/src/lib/ui/icons/edit_icon.svelteapp/web_ui/src/lib/ui/icons/folder_icon.svelteapp/web_ui/src/lib/ui/icons/folders_icon.svelteapp/web_ui/src/lib/ui/icons/key_icon.svelteapp/web_ui/src/lib/ui/icons/refresh_icon.svelteapp/web_ui/src/lib/ui/icons/shield_icon.svelteapp/web_ui/src/lib/ui/icons/terminal_icon.svelteapp/web_ui/src/lib/ui/kiln_settings_row.svelteapp/web_ui/src/routes/(app)/settings/+page.svelteapp/web_ui/src/routes/(app)/settings/page.test.ts
✅ Files skipped from review due to trivial changes (9)
- app/web_ui/src/lib/ui/icons/book_icon.svelte
- app/web_ui/src/lib/ui/icons/cube_icon.svelte
- app/web_ui/src/lib/ui/icons/key_icon.svelte
- app/web_ui/src/lib/ui/icons/folder_icon.svelte
- app/web_ui/src/lib/ui/icons/folders_icon.svelte
- app/web_ui/src/lib/ui/icons/shield_icon.svelte
- app/web_ui/src/lib/ui/icons/edit_icon.svelte
- app/web_ui/src/lib/ui/icons/terminal_icon.svelte
- app/web_ui/src/lib/ui/icons/refresh_icon.svelte
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.
these are needed for Steve's worktree_tui repo
wktool if you check out my branchsfierro/wk-selector-workspace-side-by-side