Canonical wide events for deep links, menu clicks, updater outcomes, and context comment writes - #139
Conversation
…ext comment writes Address the warning-level findings from the 2026-07-14 and 2026-07-21 wide-events audits (PRs #136/#137): - HandleDeepLink now owns a canonical deeplink.opened event emitted on every terminal outcome (completed / rejected / error) with rfa.mode, rfa.route, rfa.path_hash, and project context. - HandleMenuItemClicked now owns a canonical menu.item.clicked event with rfa.menu_id, outcome (completed / cancelled / skipped / error), and duration; sub-handlers report their outcome instead of void. - The UpdateAvailable / UpdateDownloaded listeners no longer hardcode rfa.outcome = completed in finally: a catch now flips the outcome to error (with rfa.error_class and a stable reason) before rethrowing. - Context-page comment writes own a canonical context.comment.written event (completed / rejected / skipped / error); the rejected branch keeps its diagnostic warning, whose payload keys move to snake_case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3Jts3UrUJYfdJFjvqnJZy
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds structured Laravel Context metadata and canonical logs across project opening, deep-link handling, menu actions, native updater events, and context comment writes. Tests cover completed, rejected, cancelled, skipped, partial, and error outcomes. ChangesRuntime instrumentation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HandleDeepLink
participant OpenProjectFromPathAction
participant Context
participant MainWindow
participant Log
HandleDeepLink->>Context: flush and record received URL metadata
HandleDeepLink->>OpenProjectFromPathAction: resolve project from path
OpenProjectFromPathAction->>Context: record failure metadata when resolution fails
HandleDeepLink->>Context: record outcome and route metadata
HandleDeepLink->>MainWindow: navigate to computed route
HandleDeepLink->>Log: log deeplink.opened
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f75164dc1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…nd scan failures Address Codex review on #139: OpenProjectFromPathAction and OpenRepositoryDialogAction collapse distinct failure modes into null, which the new canonical events mislabeled. - OpenProjectFromPathAction's swallowed-Throwable branch now marks the owner Context (rfa.reason = project_registration_failed + rfa.error_class), so HandleDeepLink records error instead of rejected/not_a_project for database or filesystem failures. - OpenRepositoryDialogAction marks not_a_git_repository picks and registration failures via Context; HandleMenuItemClicked maps a null project to cancelled / rejected / error accordingly. - scan-directory now reports partial (with repos_found / registered / already_tracked / failed counts) when any registration failed, and completed only for clean scans. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3Jts3UrUJYfdJFjvqnJZy
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/Listeners/HandleMenuItemClicked.php (1)
31-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider hoisting the canonical-event scaffolding into a shared helper. This
Context::flush()→$startedAt→try/catch(rethrow)/finally(outcome + duration_ms + Log::info)dance is duplicated verbatim here, inapp/Listeners/HandleDeepLink.php, and across the updater listeners inapp/Providers/NativeAppServiceProvider.php. Since this whole PR exists to make those wide events consistent, a single wrapper (e.g. arecordCanonicalEvent(string $log, Closure $body)that owns flush/timing/outcome/finally) would stop the field names and outcome semantics from quietly drifting apart the next time someone touches one copy but not the others. Purely a maintainability nicety — behavior here is correct as-is.🤖 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 `@app/Listeners/HandleMenuItemClicked.php` around lines 31 - 66, Extract the duplicated canonical-event scaffolding from HandleMenuItemClicked, HandleDeepLink, and the updater listeners in NativeAppServiceProvider into one shared helper such as recordCanonicalEvent(string $log, Closure $body). Have the helper own Context::flush(), timing, outcome initialization and finalization, error context, duration recording, and Log::info while preserving the existing outcome semantics and rethrow behavior; update each listener to execute its event-specific logic through the helper.resources/views/pages/⚡context-page.blade.php (1)
258-281: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRejected/skipped returns skip the
skipRender()optimization used on the success path.Both early returns leave
$this->commentsuntouched, so there's no structural change to render — yet only the success branch (line 286) callsskipRender(). Every add-comment attempt that gets rejected or is a no-op (e.g. a bad screen-state click, or saving a blank textarea) triggers a full component re-render for nothing.⚡ Proposed fix
\Illuminate\Support\Facades\Log::warning('context.comment.rejected', [ 'reason' => $e->reason->value, 'file_id' => $fileId, 'side' => $side, 'start_line' => $startLine, 'end_line' => $endLine, ]); + + $this->skipRender(); + return; } } if (! $comment) { $outcome = 'skipped'; + + $this->skipRender(); return; }As per path instructions,
resources/views/pages/**/*.blade.phpshould "UseskipRender()for Livewire actions whose UI is handled client-side or by targeted child updates, and do not wait for a server response before showing locally predictable visual feedback."🤖 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 `@resources/views/pages/`⚡context-page.blade.php around lines 258 - 281, Call skipRender() before both early returns in the ContextCommentRejectedException catch block and the !$comment branch, matching the existing success path behavior. Keep the outcome assignments and rejection logging unchanged.Source: Path instructions
tests/Unit/Livewire/ContextPageTest.php (1)
235-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the rejected-branch assertion and cover the still-untested
skipped/erroroutcomes.
Log::shouldHaveReceived('warning')->once();(line 249) doesn't check the message or payload, so it won't catch a regression that changescontext.comment.rejectedto something else. More importantly, this cohort'screateComment()also introducesskipped(empty body) anderror(rethrown\Throwablewithrfa.error_class/rfa.reason = comment_write_failed) outcomes — neither is exercised by any test here.✅ Suggested additions
Log::shouldHaveReceived('warning')->once()->with('context.comment.rejected', Mockery::type('array'));test('addComment records a skipped outcome when the body is empty', function () { Log::spy(); Livewire::test('pages::context-page', ['slug' => 'test-project']) ->call('addComment', 'file-1', 'file', null, null, ''); Log::shouldHaveReceived('info')->once()->with('context.comment.written'); expect(Context::get('rfa.outcome'))->toBe('skipped'); }); test('addComment records an error outcome and rethrows on unexpected failure', function () { app()->bind(ContextCommentWorkflowAction::class, fn () => new class { public function handle(mixed ...$args): never { throw new \RuntimeException('boom'); } }); Log::spy(); expect(fn () => Livewire::test('pages::context-page', ['slug' => 'test-project']) ->call('addComment', 'file-1', 'file', null, null, 'hello')) ->toThrow(\RuntimeException::class); Log::shouldHaveReceived('info')->once()->with('context.comment.written'); expect(Context::get('rfa.outcome'))->toBe('error') ->and(Context::get('rfa.reason'))->toBe('comment_write_failed'); });As per path instructions,
tests/**/*.phprequires "Every code change must be programmatically tested," and both theskippedanderrorbranches are new in this diff.🤖 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 `@tests/Unit/Livewire/ContextPageTest.php` around lines 235 - 253, Strengthen the rejected-outcome test for addComment to assert the warning uses context.comment.rejected with an array payload. Add coverage for the empty-body skipped outcome and for unexpected Throwable failures: verify the error is rethrown, context.comment.written is logged, and Context records rfa.outcome as error with rfa.reason set to comment_write_failed.Source: Path instructions
🤖 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 `@app/Listeners/HandleMenuItemClicked.php`:
- Around line 31-66: Extract the duplicated canonical-event scaffolding from
HandleMenuItemClicked, HandleDeepLink, and the updater listeners in
NativeAppServiceProvider into one shared helper such as
recordCanonicalEvent(string $log, Closure $body). Have the helper own
Context::flush(), timing, outcome initialization and finalization, error
context, duration recording, and Log::info while preserving the existing outcome
semantics and rethrow behavior; update each listener to execute its
event-specific logic through the helper.
In `@resources/views/pages/`⚡context-page.blade.php:
- Around line 258-281: Call skipRender() before both early returns in the
ContextCommentRejectedException catch block and the !$comment branch, matching
the existing success path behavior. Keep the outcome assignments and rejection
logging unchanged.
In `@tests/Unit/Livewire/ContextPageTest.php`:
- Around line 235-253: Strengthen the rejected-outcome test for addComment to
assert the warning uses context.comment.rejected with an array payload. Add
coverage for the empty-body skipped outcome and for unexpected Throwable
failures: verify the error is rethrown, context.comment.written is logged, and
Context records rfa.outcome as error with rfa.reason set to
comment_write_failed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 981a384c-21c0-4aeb-8112-6746e63df718
📒 Files selected for processing (9)
app/Actions/OpenProjectFromPathAction.phpapp/Actions/OpenRepositoryDialogAction.phpapp/Listeners/HandleDeepLink.phpapp/Listeners/HandleMenuItemClicked.phpapp/Providers/NativeAppServiceProvider.phpresources/views/pages/⚡context-page.blade.phptests/Feature/Listeners/HandleDeepLinkTest.phptests/Feature/Listeners/HandleMenuItemClickedTest.phptests/Unit/Livewire/ContextPageTest.php
… outcomes Address CodeRabbit nitpicks on #139: - createComment now calls skipRender() on the rejected and skipped early returns — neither touches $comments, so the render was pure waste. - Tighten the rejected-branch test to assert the warning event name and payload shape, and add coverage for the skipped (no comment produced) and error (rethrown Throwable) canonical outcomes. The suggestion to hoist the canonical-event try/catch/finally into a shared helper is intentionally skipped: the wide-events skill's owner pattern is written inline at each logging owner by design, matching softRefresh() and the updater listeners. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3Jts3UrUJYfdJFjvqnJZy
…ll-project mapping Apply the fgilio-review branch audit: - OpenProjectFromPathAction now marks its own null causes via Context (path_not_found, not_a_git_repository), so the deep-link and dialog flows share one reason vocabulary instead of the deep-link relabeling not-a-repo as not_a_project (kept only as the addIf fallback). - Rename outcomeForDismissedPicker to outcomeForNullProject: it maps all three null-project outcomes, not just dismissal. - Hoist the twice-computed mode and path hash in HandleDeepLink into locals shared by Context and the diagnostic breadcrumb. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3Jts3UrUJYfdJFjvqnJZy
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Unit/Livewire/ContextPageTest.php (1)
255-255: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
it()syntax and action-first descriptions.Both tests deviate from the specified Pest testing conventions. As per coding guidelines and path instructions, Pest tests should use the
it()syntax and start with action-first wording (e.g., "records" instead of "addComment records"). (Note: The arrow-function syntax guideline cannot be fully applied here since PHP does not support multi-statement arrow functions, but the action-first rule andit()syntax still apply.)
tests/Unit/Livewire/ContextPageTest.php#L255-L255: change toit('records a skipped outcome when the workflow produces no comment', function () {tests/Unit/Livewire/ContextPageTest.php#L273-L273: change toit('records an error outcome and rethrows on unexpected failure', function () {🤖 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 `@tests/Unit/Livewire/ContextPageTest.php` at line 255, Update both tests in tests/Unit/Livewire/ContextPageTest.php at lines 255-255 and 273-273 to use it() syntax and action-first descriptions: “records a skipped outcome when the workflow produces no comment” and “records an error outcome and rethrows on unexpected failure.”Sources: Coding guidelines, Path instructions
🤖 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 `@tests/Unit/Livewire/ContextPageTest.php`:
- Line 255: Update both tests in tests/Unit/Livewire/ContextPageTest.php at
lines 255-255 and 273-273 to use it() syntax and action-first descriptions:
“records a skipped outcome when the workflow produces no comment” and “records
an error outcome and rethrows on unexpected failure.”
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8bc655d4-90d4-457f-acf4-69561c242c79
📒 Files selected for processing (6)
app/Actions/OpenProjectFromPathAction.phpapp/Listeners/HandleDeepLink.phpapp/Listeners/HandleMenuItemClicked.phpresources/views/pages/⚡context-page.blade.phptests/Unit/Actions/OpenProjectFromPathActionTest.phptests/Unit/Livewire/ContextPageTest.php
🚧 Files skipped from review as they are similar to previous changes (4)
- app/Actions/OpenProjectFromPathAction.php
- resources/views/pages/⚡context-page.blade.php
- app/Listeners/HandleMenuItemClicked.php
- app/Listeners/HandleDeepLink.php
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06d3430836
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… the project A stale cached id falls back to the picker, but is_int($cachedId) still read true, so both the canonical menu.item.clicked event and the diagnostic breadcrumb attributed picker-based opens to the cache. Track whether cache resolution actually produced the project instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3Jts3UrUJYfdJFjvqnJZy
2 participants
Addresses the warning-level findings from the 2026-07-14 (#136) and 2026-07-21 (#137) wide-events audits.
Changes
HandleDeepLinknow owns a canonicaldeeplink.openedevent emitted on every terminal outcome —completed,rejected(unsupported_url/missing_path/not_a_project), orerror— withrfa.mode,rfa.route,rfa.path_hash, project id/slug, andrfa.duration_ms. Existing JSONL breadcrumbs are unchanged.HandleMenuItemClickednow owns a canonicalmenu.item.clickedevent withrfa.menu_id, outcome, and duration. Sub-handlers return their outcome (completed/cancelledwhen a dialog is dismissed) instead ofvoid; unknown ids reportskipped, throws reporterror. Project context is added toContextwhere a navigation resolves.UpdateAvailable/UpdateDownloadedinNativeAppServiceProvider) no longer hardcoderfa.outcome = 'completed'infinally: acatchflips the outcome toerrorwithrfa.error_classand a stable reason before rethrowing, matching thesoftRefresh()reference pattern.createComment) own a canonicalcontext.comment.writtenevent withcompleted/rejected/skipped/erroroutcomes plus file id, draft flag, and duration. The diagnosticcontext.comment.rejectedwarning stays on the rejected branch; its payload keys move to snake_case to match surrounding convention.Tests
HandleDeepLinkTest: canonical event + outcome/reason assertions for success, non-rfa URLs, and unresolvable paths.HandleMenuItemClickedTest: completed (open-repo), cancelled (dialog dismissed), and skipped (unknown id) outcomes.ContextPageTest: completed and rejected paths forcontext.comment.written, including the retained warning.The updater catch-path has no direct test (NativePHP
Notification::new()isn't cheaply fakeable); the logging arch tests cover the vocabulary and shape rules it must satisfy.🤖 Generated with Claude Code
https://claude.ai/code/session_01M3Jts3UrUJYfdJFjvqnJZy
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Reliability
Tests