NO-JIRA: New microcopy-review skill and context simplification
#16009
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@logonoff: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
logonoff
force-pushed
the
money
branch
2 times, most recently
from
3d40c1a to
ba68226
Compare
|
/label px-approved |
📝 WalkthroughWalkthroughThis PR updates repository documentation and AI skill descriptions: several SKILL.md files gain or change YAML front matter and content (including a new microcopy-review skill with .gitignore entry), AGENTS.md is added/rewritten, ARCHITECTURE.md and STYLEGUIDE.md are reorganized and expanded (notably adding Go and frontend patterns), and CONVENTIONS.md is removed. cursor/context.md and coderabbit.yaml are adjusted to reference AGENTS.md and remove CONVENTIONS.md from knowledge_base. Changes are documentation/configuration-only; no source code or exported API signatures were modified. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.claude/skills/pre-push-review/SKILL.md (1)
133-134:⚠️ Potential issue | 🟡 MinorUpdate the context file reference to the new source.
The doc says CodeRabbit reads.claude/CLAUDE.md, but this PR replaces that file with a symlink to AGENTS. Update the reference to avoid misleading users about where context is sourced.🔧 Suggested doc tweak
- - Note: CodeRabbit automatically reads `.claude/CLAUDE.md` for project context + - Note: CodeRabbit automatically reads the project context file (see AGENTS/AGENTS.md per repo setup)
🤖 Fix all issues with AI agents
In @.claude/skills/gen-rtl-test/SKILL.md:
- Around line 248-252: The example mock is syntactically invalid because it ends
with "return () =>" with no body; update the jest.mock('../Component', ...)
example so the factory returns a valid React component (e.g., a function
component that returns null or a simple element) while keeping the pattern
forbidden (still demonstrate the anti-pattern of requiring React in the mock).
Locate the jest.mock('../Component' ...) block and change the returned factory
from an incomplete "return () =>" to a complete component body like "return ()
=> null" (or equivalent) so the example is valid but remains a disallowed
pattern.
In @.claude/skills/test/SKILL.md:
- Around line 6-9: The argument-hint value is placed in the document body and
must live in YAML front matter for the skills parser to read it; open SKILL.md,
add or update the top YAML front matter block (between --- markers) and move the
line argument-hint: "[frontend|backend|unit <name>|all]" into that block (or
delete it if not needed) so the parser can pick it up for the /test
autocomplete; ensure the rest of the file content (like the "# /test" header)
stays outside the YAML block.
🧹 Nitpick comments (1)
.claude/skills/microcopy-review/SKILL.md (1)
86-137: Consider adding directory creation step.The IBM Style Guide extraction uses
/tmp/claude/as the output directory. Consider adding a directory creation step before the PDF extraction commands to ensure the path exists.📁 Suggested addition for directory creation
Before the pdftotext/mutool commands, add:
# Ensure output directory exists mkdir -p /tmp/claude
openshift-ci
bot
added
px-approved
logonoff
force-pushed
the
money
branch
5 times, most recently
from
0828163 to
6d2e2b7
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
logonoff
force-pushed
the
money
branch
3 times, most recently
from
b428bfc to
31384e7
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
logonoff
force-pushed
the
money
branch
4 times, most recently
from
0dfe7af to
184091a
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.claude/skills/plugin-api-review/SKILL.md:
- Around line 178-186: The markdown contains an unused reference definition
labeled "SDK release notes"; remove that reference line or add a corresponding
inline link that uses the label (e.g., [SDK release notes] somewhere in the
document) so the reference is consumed; locate the "SDK release notes"
definition among the link definitions shown alongside other references like
"[contribution guide]" and either delete the "[SDK release notes]:
../../../frontend/packages/console-dynamic-plugin-sdk/release-notes" line or
insert an inline use of "[SDK release notes]" in the content to satisfy
markdownlint.
In `@STYLEGUIDE.md`:
- Around line 47-60: The Frontend Patterns section in STYLEGUIDE.md is missing
accessibility guidance; add a new bullet under the "Frontend Patterns" list near
the existing "i18n" line that reads something like "**Accessibility**: Follow
WCAG 2.1 AA standards, use semantic HTML, ARIA labels where needed, ensure
keyboard navigation, test with screen readers" so a11y guidance is explicitly
included and grouped with i18n and other frontend best practices.
- Around line 62-81: Update the Performance section to include additional
critical patterns: document using useMemo for expensive computations (mention
symbol useMemo and example use cases like filtering/sorting large resource
lists), recommend React.memo for pure components (reference React.memo and
ListItem as an example), add guidance on list/table virtualization for large
datasets (reference VirtualizedTable or react-virtualized/react-window as
options) and explicitly warn against inline object/array literals in JSX props
(note that creating new references each render causes re-renders). Keep each
item brief with a short rationale and a one-line usage hint for useMemo,
React.memo, VirtualizedTable, and inline-literal avoidance.
- Around line 119-128: The example in STYLEGUIDE.md references a non-existent
hook useDynamicPluginInfo; replace that misleading example with real hooks from
the codebase such as useK8sWatchResource (from
`@console/internal/components/utils/k8s-watch-hook`) or useUserSettings (from
`@console/shared/src/hooks/useUserSettings`) and update the sample snippets
accordingly (e.g., show useK8sWatchResource returning [resources, loaded, error]
for a Pod list or useUserSettings returning [userSetting, setUserSetting] for a
preference) so the documented examples match actual symbols used in the repo.
🧹 Nitpick comments (8)
STYLEGUIDE.md (8)
41-41: Clarify immutability guidance to avoid confusion with deprecated Immutable.js.Line 41 recommends "immutable data structures," but line 49 notes the codebase is "migrating away from legacy Redux/Immutable.js." This could confuse contributors. Consider rewording to:
-Prefer functional programming patterns and immutable data structures +Prefer functional programming patterns and immutability (using const, spread operators, etc., not Immutable.js)This makes it clear you're advocating for language-level immutability, not the deprecated Immutable.js library.
53-55: Eliminate duplicate extension point guidance.Lines 53 and 55 both mention "console extension points for plugin integration" with nearly identical wording. Consolidate to reduce redundancy:
- **Routing**: Plugin routes go in plugin-specific route files -- **Extensions**: Use console extension points for plugin integration - **Types**: Check existing types in `console-shared` before creating new ones -- **Dynamic Plugins**: Use console extension points for plugin integration. +- **Dynamic Plugins & Extensions**: Use console extension points for plugin integration
87-91: Reconsider meta-guidance placement for AI agents.Lines 87-91 provide instructions for "AI agents" rather than human developers ("AI agents should flag use of
anytype"). While this may be intentional for AI-assisted code review, it's unconventional in a developer-facing styleguide. Consider:
- Moving AI-specific guidance to a separate
.coderabbit.yaml,.github/configuration, orAGENTS.mdfile- Rephrasing as developer-facing guidance: "Avoid the
anytype. Use proper type definitions instead."
83-103: Add K8sResourceCommon type examples for OpenShift Console context.The TypeScript type safety section should include examples specific to OpenShift Console's domain. Add guidance on using
K8sResourceCommonand typed K8s resources:// GOOD - Typed K8s resource import type { K8sResourceCommon } from '@console/internal/module/k8s'; interface PodResource extends K8sResourceCommon { spec: { containers: Array<{ name: string; image: string }>; }; } // GOOD - Type guard for narrowing function isPod(resource: K8sResourceCommon): resource is PodResource { return resource.kind === 'Pod'; }This helps developers working with K8s resources follow console conventions.
105-117: Consider adding discriminated union pattern for complex component props.The
ComponentProps<typeof X>pattern is solid for extracting types. For OpenShift Console's complex components, also document discriminated unions for variant props:// GOOD - Discriminated union for variant props type ButtonProps = | { variant: 'primary'; onClick: () => void } | { variant: 'link'; href: string } | { variant: 'danger'; onClick: () => void; confirmText: string }; // TypeScript enforces correct props for each variantThis pattern is common in console extension points and plugin components.
130-144: Enhance deprecation guidance with migration resources.The deprecation signals are accurate, but add guidance on handling deprecations:
- **Check console warnings**: Deprecated components often log warnings at runtime - **Migration path**: When using a deprecated component, consult PatternFly release notes and migration guides - **No alternative?**: If no non-deprecated alternative exists, document the usage and track for future migration - **Version awareness**: Deprecated components may be removed in future PatternFly major versions; plan upgrades accordinglyInclude a link to PatternFly's upgrade and migration documentation.
146-156: Provide nuanced barrel import guidance with tooling support.The circular dependency concern is valid, but the guidance could be more nuanced:
- When barrel imports are acceptable: Public API boundaries (e.g.,
@console/dynamic-plugin-sdk) can use barrel exports as a stable interface contract- Detection tooling: Mention ESLint plugins (
import/no-cycle) and tools likemadgeto detect circular dependencies- Why it matters: Explain that circular dependencies cause unpredictable initialization order and can lead to runtime errors (undefined values)
Example addition:
**Exception:** Barrel imports from public SDK packages (e.g., `@console/dynamic-plugin-sdk`) are acceptable as these maintain stable API boundaries. **Detection:** Use ESLint rule `import/no-cycle` and `madge` to identify circular dependencies in CI.
158-163: Expand import management with ordering and namespace import guidance.The
import typeexample is correct, but the section needs additional patterns:// GOOD - Import ordering: types first, then values import type { K8sResourceCommon, K8sModel } from '@console/internal/module/k8s'; import { k8sGet, k8sUpdate } from '@console/internal/module/k8s'; // AVOID - Namespace imports increase bundle size import * as k8sModule from '@console/internal/module/k8s'; // GOOD - Import specific utilities import { getGroupVersionKindForResource } from '@console/internal/module/k8s'; // GOOD - Default imports for components import ResourceLink from '@console/internal/components/ResourceLink';Document these conventions to improve consistency across the codebase.
logonoff
force-pushed
the
money
branch
2 times, most recently
from
cfaa3ea to
d74fd85
Compare
TheRealJon
left a comment
TheRealJon
left a comment
There was a problem hiding this comment.
/lgtm
We can merge as-is, I just had a couple of suggestions.
|
|
||
| ### Step 2: Load IBM Style Guide (Optional) | ||
|
|
||
| **IBM Style Guide status:** !`test -f .claude/skills/microcopy-review/references/ibm-style-guide.txt && echo "EXISTS - Read the file and use it for the review" || echo "NOT FOUND - Prompt the user below"` |
There was a problem hiding this comment.
We should probably refresh this at some interval. What do you think about adding another status: "old", and prompting the user to update it?
There was a problem hiding this comment.
Maybe? But it's only updated like once a year or two...
I think the cycle of re-cloning the repo every laptop replacement is a fine period to refresh this file 😁
| ```bash | ||
| find ~/Downloads -name "ibm-style-documentation.pdf" 2>/dev/null | ||
| ``` | ||
| 2. If not found, inform user and ask for the exact path |
There was a problem hiding this comment.
What if it doesn't exist and they haven't downloaded it yet? Should we provide a third option to automatically download it?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, TheRealJon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/verified by @logonoff |
openshift-ci-robot
added
the
verified
|
@logonoff: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@logonoff: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
microcopy-reviewclaude skill to help as an optional step before consulting docs for reviewCLAUDE.md/.cursor/CONTEXT.mdand just symlink to AGENTSSummary by CodeRabbit
Documentation
Chores