Repository navigation
Conversation
…, BaseModal story+tests into the kit (#14776)
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🔵 Low · up to The change adds a developer-hook check and Storybook configuration, with no runtime behavior change. The wrapper regex can let an aliased import of a different kit export pass as a wrapper, which weakens the guard in a narrow case. This is a small follow-up rather than a merge blocker. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 5.88% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 6 files. (1 skipped: 1… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the two main changes: preventing kit component forks in either app and making kit stories available in both Storybooks. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 5.88% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 6 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
|
Review (Sonnet) at 4e1da09: approve. The kit-fork guard flags a new app-local |
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
![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
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
26c87110-1e11-43e6-a4dc-1de8dce4ba5e
📒 Files selected for processing (8)
autobot-frontend/.storybook/main.jsautobot-frontend/src/components/ui/__tests__/BaseModal.test.tsautobot-frontend/src/stories/Welcome.stories.tsautobot-infrastructure/shared/scripts/hooks/pre-commit-no-cross-gui-duplicateautobot-infrastructure/shared/scripts/hooks/pre-commit-no-cross-gui-duplicate_test.pyautobot-slm-frontend/.storybook/main.jslibs/autobot-ui/src/components/BaseModal.stories.tslibs/autobot-ui/src/components/__tests__/BaseModal.test.ts
💤 Files with no reviewable changes (1)
- autobot-frontend/src/components/ui/tests/BaseModal.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
| pattern = ( | ||
| r"import\s*\{[^}]*\b" + re.escape(name) + r"\b[^}]*\}\s*from\s*['\"]" | ||
| + re.escape(KIT_PACKAGE) + r"['\"]" | ||
| ) | ||
| return re.search(pattern, text) is not None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the wrapper regex so it does not match a name inside a different import statement.
The pattern import\s*\{[^}]*\bNAME\b[^}]*\}\s*from\s*['\"]@autobot/ui['\"] matches an import that lists NAME as an alias target. An example is import { Other as SkipLink } from '@autobot/ui'. The file then counts as a wrapper although it does not import the kit component. The pattern also matches import type { NAME } only if the type keyword is absent. The PR notes already flag that an export { X } from re-export is a false positive in the other direction.
Match the name as a complete import specifier. Reject an as alias that renames a different export to NAME.
Proposed fix
--- "a/autobot-infrastructure/shared/scripts/hooks/pre-commit-no-cross-gui-duplicate"
+++ "b/autobot-infrastructure/shared/scripts/hooks/pre-commit-no-cross-gui-duplicate"
@@ -216,10 +216,10 @@
text = path.read_text(encoding="utf-8")
except OSError:
return False
pattern = (
- r"import\s*\{[^}]*\b" + re.escape(name) + r"\b[^}]*\}\s*from\s*['\"]"
+ r"import\s*\{[^}]*(?:[{,]\s*)" + re.escape(name) + r"\s*(?=[,}])[^}]*\}\s*from\s*['\"]"
+ re.escape(KIT_PACKAGE) + r"['\"]"
)
return re.search(pattern, text) is not None
| kit_violations = collect_kit_fork_violations(repo_root) | ||
|
|
||
| vacuity_exit = _check_vacuity(comp_scanned, vocab_scanned, len(kit_component_names(repo_root))) | ||
| if vacuity_exit is not None: | ||
| return vacuity_exit | ||
|
|
||
| violations = comp_violations + type_violations | ||
| violations = comp_violations + type_violations + kit_violations | ||
| if not violations: | ||
| print(f"No cross-GUI duplicates found ({comp_scanned} composables, {vocab_scanned} utils/types files scanned).") | ||
| print(f"No cross-GUI duplicates found ({comp_scanned} composables, {vocab_scanned} utils/types files scanned, " | ||
| f"{len(kit_component_names(repo_root))} kit components checked).") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Compute kit_component_names(repo_root) once.
The function runs three times in main (once inside collect_kit_fork_violations, then at Lines 299 and 306). Each call globs the kit directory again. Compute the set once and pass it down. This also keeps the vacuity count and the scanned names consistent.
1 participant
Refs #14776
Refs #18241
Single-issue rationale
This is slice S0 of Refined UI step 3 (#18241). It adds the guard and the Storybook wiring that every later one-family-per-PR consolidation slice relies on. #14776 stays open: its forks (BaseButton, BaseBadge, BaseCard, EmptyState) are retired by later slices, and this PR only stops new ones.
Thinking Path
The owner rule is one canonical implementation per concept.
@autobot/uiis the home for shared primitives, and BaseModal already proves the route (60 main-app importers, no local copy). Before consolidating each family, a guard has to stop a new fork appearing while the old ones are retired.The existing hook
pre-commit-no-cross-gui-duplicatealready guards composable filenames and util/type names across both GUIs. This PR extends it to.vuecomponents rather than adding a parallel guard.What Changed
Guard:
autobot-infrastructure/shared/scripts/hooks/pre-commit-no-cross-gui-duplicategains a third check..vuefile in either app'ssrc/whose basename matches a kit component (libs/autobot-ui/src/components/*.vue) is rejected unless it is a thin wrapper importing that same name from@autobot/ui. The SLMSkipLink.vueis the precedent.KIT_FORK_GRANDFATHER): keyed per path, shrink-only and checked both ways. An entry whose file is gone, or has become a wrapper, fails as stale. It covers today's four forks, each verified to have no@autobot/uiimport:autobot-frontend/src/components/base/BaseButton.vueautobot-frontend/src/components/base/BaseBadge.vueautobot-frontend/src/components/base/BaseCard.vueautobot-frontend/src/components/ui/EmptyState.vueStorybook: both apps'
.storybook/main.jsadd'../../libs/autobot-ui/src/**/*.stories.ts', so kit stories render under each app's tokens.BaseModal story and tests moved into the kit:
autobot-frontend/src/components/ui/BaseModal.stories.tsbecomeslibs/autobot-ui/src/components/BaseModal.stories.ts(titleKit/BaseModal), andWelcome.stories.tsis updated.BaseModal.test.ts.Behaviour delta: none at runtime.
Verification
No cross-GUI duplicates found (262 composables, 69 utils/types files scanned, 6 kit components checked).BaseModal.test.tsincluded).npm run type-check(autobot-frontend): clean.autobot-slm-frontendtype-check did not run: the shared SLMnode_moduleshas novue-tsc. The only SLM change is.storybook/main.js.AUTOBOT_PREPUSH_ALLOW_TIMEOUT=1andAUTOBOT_PREPUSH_ALLOW_UNRUNNABLE=1, owner-approved.npm run type-checkabove was clean.Not done here: the
_NOTIFICATION_ENTRY_POINTSover-count (it counts a workflow config composable as a toast system). It lives inrepo_tests/frontend_fragmentation_ratchet_test.py:193, which open PR #18373 also edits, so it follows after #18373 merges.Model Used
Claude Opus 5.5 (coordinating). Sonnet subagents did the planning and the implementation.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation