Add EventType info to ElementInvalidationEvent#40
Open
jdreesen wants to merge 9 commits into
Open
Conversation
WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Element as Element (Pimcore)
participant Listener as InvalidateElementListener
participant Event as ElementInvalidationEvent
participant Dispatcher as EventDispatcher
alt Update Operation
Element->>Listener: onUpdate(event)
Listener->>Listener: invalidateElement(element, EventType::Update)
else Delete Operation
Element->>Listener: onDelete(event)
Listener->>Listener: invalidateElement(element, EventType::Delete)
end
Listener->>Event: ElementInvalidationEvent::fromElement(element, type)
Event->>Event: __construct(type, element, elementType, cacheTags)
Listener->>Dispatcher: dispatch(event)
Dispatcher->>Dispatcher: Event contains type information
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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)
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 |
Provides codebase overview, conventions, architecture patterns, and development commands to streamline AI-assisted development. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Rules (.claude/rules/): - code-style: PHP conventions with examples from codebase - testing: PHPUnit + Prophecy patterns and conventions - architecture: Decorator chains, adapters, lifecycle flows - static-analysis: PHPStan level 8 requirements - bundle-usage: How the bundle works for developers Skills (.claude/skills/): - php-best-practices: PHP 8.x audit with 45+ rules - php-pro: Senior PHP dev with framework reference guides - tdd: Test-driven development workflow - debug: Systematic root-cause debugging - code-review: Handle review feedback with technical rigor - brainstorm: Ideas to designs to specs - web-search: Web search & extraction via inference.sh - writing-skills: TDD for skill documentation - humanizer: Remove AI writing patterns Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- /review skill with systematic 8-category checklist: dead code, architecture, documentation, tests, security, performance, BC breaks, static analysis - Detailed checklist with project-specific checks - roles.md rule defining writer vs reviewer behavior - Severity guide: BLOCKER / WARNING / SUGGESTION Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Documents how to use Claude Code with this project: - Rules vs skills explained - Writer/reviewer workflow - When and how to update CLAUDE.md, rules, and skills - Available skills reference - Tips for effective usage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Queries Context7 API for current docs instead of relying on training data. No API key needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Warnings fixed: - Add /context7 to developer guide skills table and structure - Fix decorator chain in CLAUDE.md to show execution order - Remove misleading "(maximum)" from PHPStan level 8 Suggestions fixed: - Use named argument in CacheTags::fromStrings() example - Clarify /review vs /code-review in skills table - Document \assert() usage in testing.md - Add project context note to php-pro PHPStan reference Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix blank line after <?php in EventType.php - Rename $collectTagsResponseTagger to $traceableResponseTagger in TraceableResponseTaggerTest - Fix broken fromElement() calls in InvalidateElementListenerTest (missing EventType arg) - Add tests verifying EventType::Update and EventType::Delete are dispatched correctly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Unit/Cache/ResponseTagger/TraceableResponseTaggerTest.php (1)
17-26:⚠️ Potential issue | 🔴 CriticalIncomplete property rename causes test failures.
The property was renamed to
$traceableResponseTaggeron line 17, but all usages throughout the file still reference the old name$collectTagsResponseTagger. This includes:
- Line 25:
setUp()assigns to the old property name- Lines 33, 41, 55, 65, 71, 74: Test methods use the old property name
The tests will fail or behave unexpectedly because
$traceableResponseTaggeris never assigned.🐛 Proposed fix: complete the rename
protected function setUp(): void { $this->innerTagger = $this->prophesize(ResponseTagger::class); - $this->collectTagsResponseTagger = new TraceableResponseTagger($this->innerTagger->reveal()); + $this->traceableResponseTagger = new TraceableResponseTagger($this->innerTagger->reveal()); }And update all test method usages from
$this->collectTagsResponseTaggerto$this->traceableResponseTagger:public function tag_should_collect_tags(): void { - $this->collectTagsResponseTagger->tag( + $this->traceableResponseTagger->tag( new CacheTags( CacheTag::fromString('tag1'), CacheTag::fromString('tag2'), )); self::assertSame( 'tag1,tag2', - $this->collectTagsResponseTagger->recordedTags->toString(), + $this->traceableResponseTagger->recordedTags->toString(), ); }Apply the same change to
tag_should_forward_tags_to_inner_tagger(line 55) andreset_should_reset_collected_tags(lines 65, 71, 74).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Cache/ResponseTagger/TraceableResponseTaggerTest.php` around lines 17 - 26, The property was partially renamed to $traceableResponseTagger but setUp() and all test methods still reference $collectTagsResponseTagger; update setUp() to assign $this->traceableResponseTagger = new TraceableResponseTagger($this->innerTagger->reveal()) and replace every occurrence of $this->collectTagsResponseTagger in test methods (including tag_should_forward_tags_to_inner_tagger and reset_should_reset_collected_tags) with $this->traceableResponseTagger so the instantiated object is used consistently.
🧹 Nitpick comments (8)
.claude/skills/brainstorm/SKILL.md (1)
52-52: Use “multiple-choice” for consistency and grammar polish.At Line 52, hyphenating the compound adjective improves readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/brainstorm/SKILL.md at line 52, Replace the compound adjective "multiple choice" with the hyphenated form "multiple-choice" in the SKILL.md text where the phrase "multiple choice questions" appears; update the sentence to read "multiple-choice questions" to improve consistency and grammar..claude/skills/php-best-practices/rules/modern-enums.md (1)
82-87: Clarify the$tag->type->typeproperty access.The expression
$tag->type->typeaccesses a nestedtypeproperty twice, which appears confusing or potentially erroneous. If$tag->typealready returns anElementType, it should be used directly.📝 Suggested fix
-return match ($tag->type->type) { +return match ($tag->type) { ElementType::Asset => $this->asset->isEnabled($tag), ElementType::Document => $this->document->isEnabled($tag), ElementType::Object => $this->object->isEnabled($tag), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/php-best-practices/rules/modern-enums.md around lines 82 - 87, The match expression is using a confusing double property access `$tag->type->type`; update the code to use the actual ElementType value directly (e.g. `match ($tag->type)`) or otherwise dereference the correct property so the match operates on an ElementType enum instance; also add a defensive check or typehint where `type` is assigned to ensure `$tag->type` is an ElementType before calling methods like `isEnabled` on `$this->asset`, `$this->document`, and `$this->object`..claude/skills/php-best-practices/rules/solid-interface-segregation.md (2)
68-71: Same undefined$tagsissue in the "Good" example.For consistency and clarity, show where
$tagsoriginates.📝 Suggested fix
public function onUpdate(): void { + $tags = $this->collectTags(); $this->invalidator->invalidate($tags); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/php-best-practices/rules/solid-interface-segregation.md around lines 68 - 71, The onUpdate() method in the "Good" example uses an undefined $tags variable; update onUpdate() so $tags is defined or passed in — e.g., add a parameter to onUpdate(array $tags): void or obtain $tags from a known source (like $this->getTags() or a method call) before calling $this->invalidator->invalidate($tags); ensure the change is applied to the onUpdate() declaration and any call sites so $tags is clearly sourced.
33-37: Undefined variable$tagsin example.The
$tagsvariable is used but not defined within theonUpdatemethod scope, which could confuse readers. Consider showing where$tagscomes from.📝 Suggested fix
public function onUpdate(): void { - $this->cache->invalidate($tags); // Only uses 1 of 5 methods + $tags = $this->collectTags(); // Example: fetch tags from somewhere + $this->cache->invalidate($tags); // Only uses 1 of 5 methods }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/php-best-practices/rules/solid-interface-segregation.md around lines 33 - 37, The example's onUpdate method uses an undefined local variable $tags when calling $this->cache->invalidate($tags); — fix by showing where $tags comes from: either add a method parameter (public function onUpdate(array $tags): void) and use that, reference an existing class property (e.g. $this->tags) initialized elsewhere, or construct/derive $tags inside onUpdate before calling $this->cache->invalidate; update the example to include one of these approaches so $tags is clearly defined for the onUpdate method and matches the CacheInterface::invalidate usage..claude/skills/debug/SKILL.md (1)
81-81: Add language specifier to fenced code block.The fenced code block at line 81 is missing a language identifier, which affects syntax highlighting and rendering.
📝 Suggested fix
-``` +```plaintext For EACH component boundary:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/debug/SKILL.md at line 81, The fenced code block containing the line "For EACH component boundary:" is missing a language specifier; update the opening fence for that block (the triple backticks before "For EACH component boundary:") to include a language identifier such as plaintext (e.g., change ``` to ```plaintext) so the block is properly labeled for syntax highlighting and rendering..claude/skills/tdd/SKILL.md (1)
49-58: Add language specifiers to fenced code blocks.Multiple fenced code blocks are missing language identifiers, which affects rendering and syntax highlighting:
- Line 49: Process flow diagram
- Line 251: Test output
- Line 269: Test output
📝 Suggested fixes
-``` +```text RED (Write failing test)-``` +```text $ composer tests -- --filter it_rejects_empty_cache_tag-``` +```text $ composer tests -- --filter it_rejects_empty_cache_tagAlso applies to: 251-254, 269-272
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/tdd/SKILL.md around lines 49 - 58, Several fenced code blocks in .claude/skills/tdd/SKILL.md are missing language specifiers (e.g., the flow block starting with "RED (Write failing test)" and the test output blocks containing "$ composer tests -- --filter it_rejects_empty_cache_tag"); update each triple-backtick fence to include an appropriate language tag such as text (e.g., ```text) so the process flow and test output blocks render with correct syntax highlighting and formatting for the blocks containing "RED (Write failing test)" and the test output lines.doc/11-claude-code.md (1)
15-38: Consider adding language identifiers to fenced code blocks.Markdownlint flags several code blocks without language specifiers. For conversation/workflow examples, you can use
textor leave them as-is since they represent human-AI dialog rather than executable code. The directory tree at line 15 could benefit fromtextorplaintextfor consistency.Example fix for the directory structure block
-``` +```text CLAUDE.md # Project overview, quick reference, conventions .claude/ ├── rules/ # Always loaded — project conventionsAlso applies to: 52-59, 76-82, 88-95, 101-107, 164-167, 171-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/11-claude-code.md` around lines 15 - 38, Add language identifiers to the fenced code blocks in the CLAUDE.md document—at minimum label the directory tree block that starts with "CLAUDE.md" and ".claude/" as ```text (or ```plaintext) and similarly tag other example/conversation/workflow blocks (e.g., the conversation/workflow examples and other unlabeled snippets) so markdownlint stops flagging them; update the fenced code delimiters throughout the file (including the directory tree and other example blocks referenced in the review) to include the chosen language token..claude/skills/writing-skills/SKILL.md (1)
209-214: Consider using consistent skill name format in cross-references.The examples reference skills as "tdd skill" and "debug skill" (lowercase, no hyphens), but the naming convention (line 91) requires "letters, numbers, and hyphens only", and other examples show hyphenated names like "condition-based-waiting" and "root-cause-tracing".
For clarity and consistency, consider using the actual skill names (e.g., if the TDD skill is named
test-driven-development, reference it that way).📝 Example of consistent formatting
-**REQUIRED SUB-SKILL:** Use tdd skill -**REQUIRED BACKGROUND:** You MUST understand debug skill +**REQUIRED SUB-SKILL:** Use test-driven-development skill +**REQUIRED BACKGROUND:** You MUST understand debugging skillOr if the skill names are simply
tddanddebug:-**REQUIRED SUB-SKILL:** Use tdd skill -**REQUIRED BACKGROUND:** You MUST understand debug skill +**REQUIRED SUB-SKILL:** Use tdd +**REQUIRED BACKGROUND:** You MUST understand debug🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/writing-skills/SKILL.md around lines 209 - 214, Update the inconsistent cross-reference texts "tdd skill" and "debug skill" to match the repository naming convention (letters, numbers, and hyphens only) used elsewhere (see examples "condition-based-waiting" and "root-cause-tracing") — replace "tdd skill" with the canonical skill name (e.g., "test-driven-development" or "tdd" as appropriate) and "debug skill" with the canonical "debug" name, and ensure the "REQUIRED SUB-SKILL" and "REQUIRED BACKGROUND" lines use the exact hyphenated identifiers instead of plain phrases so cross-references are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/brainstorm/SKILL.md:
- Around line 33-42: The fenced code block that begins with "Explore project
context" is missing a language tag (triggers MD040); fix it by adding a language
identifier (e.g., "text") to the opening triple backticks so the block becomes
```text ... ```; update the block in SKILL.md around the "Explore project
context" section to include this tag to satisfy markdownlint.
In @.claude/skills/debug/condition-based-waiting.md:
- Around line 20-27: The "good" example loop using $maxAttempts, $interval,
$result and $cache->get('key') silently exits on timeout; update it to
explicitly handle the timeout after the loop (e.g., check if $result is still
null and then throw an exception, return an error, or log a clear timeout
message) so callers see the failure; apply the same explicit post-loop timeout
handling to the other example using the same variables.
In @.claude/skills/review/SKILL.md:
- Around line 14-19: The two untyped fenced code blocks that start with the
lines "1. SCOPE: Understand what changed and why" and the later block beginning
with "## Review Summary" should include language identifiers to satisfy MD040;
update the first fence to use a language like ```text (or ```markdown) and
update the second fenced block to use ```markdown (or ```text) so both fences
become typed, leaving the inner content unchanged and preserving indentation and
content in the blocks.
---
Outside diff comments:
In `@tests/Unit/Cache/ResponseTagger/TraceableResponseTaggerTest.php`:
- Around line 17-26: The property was partially renamed to
$traceableResponseTagger but setUp() and all test methods still reference
$collectTagsResponseTagger; update setUp() to assign
$this->traceableResponseTagger = new
TraceableResponseTagger($this->innerTagger->reveal()) and replace every
occurrence of $this->collectTagsResponseTagger in test methods (including
tag_should_forward_tags_to_inner_tagger and reset_should_reset_collected_tags)
with $this->traceableResponseTagger so the instantiated object is used
consistently.
---
Nitpick comments:
In @.claude/skills/brainstorm/SKILL.md:
- Line 52: Replace the compound adjective "multiple choice" with the hyphenated
form "multiple-choice" in the SKILL.md text where the phrase "multiple choice
questions" appears; update the sentence to read "multiple-choice questions" to
improve consistency and grammar.
In @.claude/skills/debug/SKILL.md:
- Line 81: The fenced code block containing the line "For EACH component
boundary:" is missing a language specifier; update the opening fence for that
block (the triple backticks before "For EACH component boundary:") to include a
language identifier such as plaintext (e.g., change ``` to ```plaintext) so the
block is properly labeled for syntax highlighting and rendering.
In @.claude/skills/php-best-practices/rules/modern-enums.md:
- Around line 82-87: The match expression is using a confusing double property
access `$tag->type->type`; update the code to use the actual ElementType value
directly (e.g. `match ($tag->type)`) or otherwise dereference the correct
property so the match operates on an ElementType enum instance; also add a
defensive check or typehint where `type` is assigned to ensure `$tag->type` is
an ElementType before calling methods like `isEnabled` on `$this->asset`,
`$this->document`, and `$this->object`.
In @.claude/skills/php-best-practices/rules/solid-interface-segregation.md:
- Around line 68-71: The onUpdate() method in the "Good" example uses an
undefined $tags variable; update onUpdate() so $tags is defined or passed in —
e.g., add a parameter to onUpdate(array $tags): void or obtain $tags from a
known source (like $this->getTags() or a method call) before calling
$this->invalidator->invalidate($tags); ensure the change is applied to the
onUpdate() declaration and any call sites so $tags is clearly sourced.
- Around line 33-37: The example's onUpdate method uses an undefined local
variable $tags when calling $this->cache->invalidate($tags); — fix by showing
where $tags comes from: either add a method parameter (public function
onUpdate(array $tags): void) and use that, reference an existing class property
(e.g. $this->tags) initialized elsewhere, or construct/derive $tags inside
onUpdate before calling $this->cache->invalidate; update the example to include
one of these approaches so $tags is clearly defined for the onUpdate method and
matches the CacheInterface::invalidate usage.
In @.claude/skills/tdd/SKILL.md:
- Around line 49-58: Several fenced code blocks in .claude/skills/tdd/SKILL.md
are missing language specifiers (e.g., the flow block starting with "RED (Write
failing test)" and the test output blocks containing "$ composer tests --
--filter it_rejects_empty_cache_tag"); update each triple-backtick fence to
include an appropriate language tag such as text (e.g., ```text) so the process
flow and test output blocks render with correct syntax highlighting and
formatting for the blocks containing "RED (Write failing test)" and the test
output lines.
In @.claude/skills/writing-skills/SKILL.md:
- Around line 209-214: Update the inconsistent cross-reference texts "tdd skill"
and "debug skill" to match the repository naming convention (letters, numbers,
and hyphens only) used elsewhere (see examples "condition-based-waiting" and
"root-cause-tracing") — replace "tdd skill" with the canonical skill name (e.g.,
"test-driven-development" or "tdd" as appropriate) and "debug skill" with the
canonical "debug" name, and ensure the "REQUIRED SUB-SKILL" and "REQUIRED
BACKGROUND" lines use the exact hyphenated identifiers instead of plain phrases
so cross-references are consistent.
In `@doc/11-claude-code.md`:
- Around line 15-38: Add language identifiers to the fenced code blocks in the
CLAUDE.md document—at minimum label the directory tree block that starts with
"CLAUDE.md" and ".claude/" as ```text (or ```plaintext) and similarly tag other
example/conversation/workflow blocks (e.g., the conversation/workflow examples
and other unlabeled snippets) so markdownlint stops flagging them; update the
fenced code delimiters throughout the file (including the directory tree and
other example blocks referenced in the review) to include the chosen language
token.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
.claude/rules/architecture.md.claude/rules/bundle-usage.md.claude/rules/code-style.md.claude/rules/roles.md.claude/rules/static-analysis.md.claude/rules/testing.md.claude/skills/brainstorm/SKILL.md.claude/skills/code-review/SKILL.md.claude/skills/context7/SKILL.md.claude/skills/debug/SKILL.md.claude/skills/debug/condition-based-waiting.md.claude/skills/debug/defense-in-depth.md.claude/skills/debug/root-cause-tracing.md.claude/skills/humanizer/SKILL.md.claude/skills/php-best-practices/SKILL.md.claude/skills/php-best-practices/rules/error-custom-exceptions.md.claude/skills/php-best-practices/rules/modern-constructor-promotion.md.claude/skills/php-best-practices/rules/modern-enums.md.claude/skills/php-best-practices/rules/modern-first-class-callables.md.claude/skills/php-best-practices/rules/modern-match-expression.md.claude/skills/php-best-practices/rules/modern-readonly-properties.md.claude/skills/php-best-practices/rules/solid-dependency-inversion.md.claude/skills/php-best-practices/rules/solid-interface-segregation.md.claude/skills/php-best-practices/rules/solid-single-responsibility.md.claude/skills/php-best-practices/rules/type-strict-mode.md.claude/skills/php-pro/SKILL.md.claude/skills/php-pro/references/async-patterns.md.claude/skills/php-pro/references/laravel-patterns.md.claude/skills/php-pro/references/modern-php-features.md.claude/skills/php-pro/references/symfony-patterns.md.claude/skills/php-pro/references/testing-quality.md.claude/skills/review/SKILL.md.claude/skills/review/checklist.md.claude/skills/tdd/SKILL.md.claude/skills/web-search/SKILL.md.claude/skills/writing-skills/SKILL.mdCLAUDE.mdREADME.mddoc/11-claude-code.mdsrc/Element/EventType.phptests/Unit/Cache/ResponseTagger/TraceableResponseTaggerTest.phptests/Unit/Element/InvalidateElementListenerTest.php
✅ Files skipped from review due to trivial changes (7)
- .claude/skills/debug/defense-in-depth.md
- .claude/skills/debug/root-cause-tracing.md
- README.md
- .claude/skills/php-pro/references/laravel-patterns.md
- .claude/rules/testing.md
- .claude/skills/php-pro/references/testing-quality.md
- .claude/skills/php-best-practices/rules/error-custom-exceptions.md
Comment on lines
+33
to
+42
| ``` | ||
| Explore project context | ||
| → Ask clarifying questions | ||
| → Propose 2-3 approaches | ||
| → Present design sections | ||
| → User approves design? | ||
| → no, revise → Present design sections | ||
| → yes → Write design doc | ||
| → Invoke writing-plans skill | ||
| ``` |
There was a problem hiding this comment.
Specify a language for fenced code blocks to satisfy markdownlint.
The fence beginning at Line 33 has no language tag (MD040), which will keep lint warnings active.
Minimal lint fix
-```
+```text
Explore project context
→ Ask clarifying questions
→ Propose 2-3 approaches
→ Present design sections
→ User approves design?
→ no, revise → Present design sections
→ yes → Write design doc
→ Invoke writing-plans skill</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/brainstorm/SKILL.md around lines 33 - 42, The fenced code
block that begins with "Explore project context" is missing a language tag
(triggers MD040); fix it by adding a language identifier (e.g., "text") to the
opening triple backticks so the block becomes text ... ; update the block
in SKILL.md around the "Explore project context" section to include this tag to
satisfy markdownlint.
</details>
<!-- fingerprinting:phantom:triton:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+20
to
+27
| for ($i = 0; $i < $maxAttempts; $i++) { | ||
| $result = $cache->get('key'); | ||
| if (null !== $result) { | ||
| break; | ||
| } | ||
| usleep($interval); | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Add explicit timeout failure in the “good” example.
The sample loop at Line 20–Line 27 does not demonstrate the timeout failure behavior required by Line 39, which may normalize silent timeouts.
Suggested doc snippet update
$maxAttempts = 50;
$interval = 100_000; // 100ms in microseconds
+$result = null;
for ($i = 0; $i < $maxAttempts; $i++) {
$result = $cache->get('key');
if (null !== $result) {
break;
@@
usleep($interval);
}
+
+if (null === $result) {
+ throw new \RuntimeException('Timed out waiting for cache key "key" after 5s');
+}Also applies to: 35-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/debug/condition-based-waiting.md around lines 20 - 27, The
"good" example loop using $maxAttempts, $interval, $result and
$cache->get('key') silently exits on timeout; update it to explicitly handle the
timeout after the loop (e.g., check if $result is still null and then throw an
exception, return an error, or log a clear timeout message) so callers see the
failure; apply the same explicit post-loop timeout handling to the other example
using the same variables.
Comment on lines
+14
to
+19
| ``` | ||
| 1. SCOPE: Understand what changed and why | ||
| 2. CHECK: Run through all review categories | ||
| 3. REPORT: Output findings with severity and locations | ||
| 4. SUGGEST: Provide concrete fixes, not vague advice | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks (MD040).
Both fences starting at Line 14 and Line 48 are untyped, which triggers markdownlint warnings.
Minimal lint fix
-```
+```text
1. SCOPE: Understand what changed and why
2. CHECK: Run through all review categories
3. REPORT: Output findings with severity and locations
4. SUGGEST: Provide concrete fixes, not vague advice- +markdown
Review Summary
X blocker(s), Y warning(s), Z suggestion(s)
Blockers
src/File.php:42— [category] Description. Fix: ...
@@- Test coverage
...
Also applies to: 48-67
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 14-14: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/review/SKILL.md around lines 14 - 19, The two untyped fenced
code blocks that start with the lines "1. SCOPE: Understand what changed and
why" and the later block beginning with "## Review Summary" should include
language identifiers to satisfy MD040; update the first fence to use a language
like ```text (or ```markdown) and update the second fenced block to use
```markdown (or ```text) so both fences become typed, leaving the inner content
unchanged and preserving indentation and content in the blocks.
Contributor
|
I’m not a fan of the MR title. It explains the changes, but it doesn’t clearly describe the feature. |
Contributor
|
I created a brach, with testcases and documentation: for this MR: |
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
2 participants
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.
Sometimes you may need the information whether the element was updated or deleted.
Summary by CodeRabbit