fix: align Strix budget self-test and UI terminology - #348
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
PR governance metadata gate is not ready for
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (5)
📝 WalkthroughWalkthroughThis PR updates Strix workflow CI budgeting by removing a PR-evidence conditional and increasing timeout values, and refactors frontend terminology across multiple components to use "relationship context" and "context synthesis" instead of "network graph" and "summary". ChangesStrix workflow budget simplification
Frontend terminology refactoring
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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: 2
🧹 Nitpick comments (1)
.jules/palette.md (1)
3-3: ⚡ Quick winAvoid codifying
it.skipas the default test update strategy.This guidance should prefer rewriting/removing obsolete assertions, not skipping coverage.
Also applies to: 7-7
🤖 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 @.jules/palette.md at line 3, Update the guidance text to remove any implication that using it.skip is the default remediation for failing tests; instead instruct maintainers to prefer rewriting or deleting obsolete assertions and stubbing only when necessary. Replace the phrase that codifies "it.skip" as the default strategy with explicit guidance: search for rigid assertions targeting the "Developer tab" or internal/admin components, update or remove those assertions, and only use it.skip or stubs as a last resort with a clear TODO and link to the owning ticket. Ensure the updated wording applies to all occurrences (including the referenced duplicate at 7-7) and explicitly calls out not to reduce test coverage by skipping tests by default.
🤖 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 @.jules/palette.md:
- Around line 5-7: Remove the duplicated refactor note block that repeats the
heading "2026-06-04 - Settings UI Developer Tab Refactor" and its Action/
Learning lines, leaving a single canonical instance; ensure the remaining
content still mentions the "Developer' tab" guidance and run the linter to
confirm duplicate-heading warnings are gone; also search for any tests that
rigidly assert Developer tab structure and change them to stub/it.skip or make
assertions tolerant to one instance only.
In `@frontend/src/components/SettingsLayout.test.tsx`:
- Line 235: Re-enable the skipped tests in SettingsLayout.test.tsx (e.g., the
"renders the self-hosted connector manifest and keeps mobile settings tabs
reachable" test and the other two skipped cases) or replace them with
non-skipped equivalents that still assert signed-session behavior: mock and
assert that requests to signed backend routes carry Authorization: Bearer
<naruon_session_token> (exercise the naruon_session_token path), verify
forbidden identity headers are absent, keep tabnabbing/hardening assertions for
external links, and update UI expectations to assert the Developer tab/links are
absent rather than present; update test helpers/mocks used by the SettingsLayout
component tests to reflect these expectations.
---
Nitpick comments:
In @.jules/palette.md:
- Line 3: Update the guidance text to remove any implication that using it.skip
is the default remediation for failing tests; instead instruct maintainers to
prefer rewriting or deleting obsolete assertions and stubbing only when
necessary. Replace the phrase that codifies "it.skip" as the default strategy
with explicit guidance: search for rigid assertions targeting the "Developer
tab" or internal/admin components, update or remove those assertions, and only
use it.skip or stubs as a last resort with a clear TODO and link to the owning
ticket. Ensure the updated wording applies to all occurrences (including the
referenced duplicate at 7-7) and explicitly calls out not to reduce test
coverage by skipping tests by default.
🪄 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: 68d346cb-d6f3-4861-a2c6-b06e0fbaff43
📒 Files selected for processing (2)
.jules/palette.mdfrontend/src/components/SettingsLayout.test.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/strix.yml (1)
314-317: ⚡ Quick winRemove redundant conditional setting identical values.
Both the default assignment (line 314) and the PR evidence branch (line 316) set
process_budget_secondsto the same value (2400). The conditional is now pointless and should be removed for clarity.♻️ Simplify by removing the redundant conditional
- process_budget_seconds="2400" - if [ "$IS_PR_EVIDENCE_RUN" = "true" ]; then - process_budget_seconds="2400" - fi + process_budget_seconds="2400"🤖 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 @.github/workflows/strix.yml around lines 314 - 317, The conditional block that checks IS_PR_EVIDENCE_RUN is redundant because process_budget_seconds is already initialized to 2400; remove the entire if block that references IS_PR_EVIDENCE_RUN so only the single assignment process_budget_seconds="2400" remains, leaving no change in behavior but improving clarity.
🤖 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.
Nitpick comments:
In @.github/workflows/strix.yml:
- Around line 314-317: The conditional block that checks IS_PR_EVIDENCE_RUN is
redundant because process_budget_seconds is already initialized to 2400; remove
the entire if block that references IS_PR_EVIDENCE_RUN so only the single
assignment process_budget_seconds="2400" remains, leaving no change in behavior
but improving clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 25adc5ce-ab52-4dfd-9804-6271eccbe86f
📒 Files selected for processing (2)
.github/workflows/strix.yml.jules/palette.md
✅ Files skipped from review due to trivial changes (1)
- .jules/palette.md
seonghobae
changed the title
1 participant
Align the Strix workflow self-test with the current 120-minute / 3600-second / 7200-second budget contract, remove the redundant PR evidence budget reassignment, and clean up remaining UI terminology drift against docs/ui-ux.\n\nValidation:\n- bash -n scripts/ci/strix_quick_gate.sh scripts/ci/test_strix_quick_gate.sh\n- git diff --check\n- frontend: npm test -- src/components/DashboardLayout.test.tsx src/app/search/page.test.tsx src/components/NetworkGraph.test.tsx src/components/SettingsLayout.test.tsx\n\nNote: local full scripts/ci/test_strix_quick_gate.sh progressed past the previous static assertion failure but ran into the long fake Strix case matrix without output, so it was stopped locally and should be left to CI.
Summary by CodeRabbit