ci(claude-review): replace plugin invocation with inline gh pr comment#71
Merged
ci(claude-review): replace plugin invocation with inline gh pr comment#71
Conversation
The Claude Code Review workflow on this repo had not been posting review comments on any PR — the workflow ran successfully (job returned exit 0) but no comment ever appeared. Investigation on #70 found two contributing problems: 1. The workflow used a plugin-based prompt invocation (`/code-review:code-review` via the `code-review@claude-code-plugins` plugin from the `anthropics/claude-code` marketplace) that silently fails to post review comments. The plugin appears to either suppress its output entirely or use an internal posting path that is not wired up correctly with the GitHub App token context the action provides. 2. The Claude Code Action validates the workflow file against the default branch's copy as a security mitigation, so any in-PR fix to this workflow itself fails with a 401 token-exchange error ("Workflow validation failed. The workflow file must exist and have identical content to the version on the repository's default branch"). This means the fix has to land on main first via a dedicated PR (this one) before it takes effect on any other PR. The livetemplate library repo's claude-code-review.yml uses a different approach that posts comments reliably: - Inline prompt that explicitly tells Claude to call `gh pr comment` via its Bash tool - `claude_args` allow-list whitelisting the relevant gh CLI subcommands (gh issue view, gh search, gh issue list, gh pr comment, gh pr diff, gh pr view, gh pr list) - `allowed_bots: copilot-swe-agent` to interact with Copilot comments on the same PR With that wiring, Claude generates the review, runs `gh pr comment` itself (using the GH App installation token that the Claude Code Action injects into the runner's environment), and the comment shows up on the PR within 5–8 minutes. This has been working on the library repo continuously, producing 4 review comments across the recent rounds of the Session.TriggerAction PR (livetemplate/livetemplate#336). Changes in this PR: - Drop `plugin_marketplaces`, `plugins`, and the slash-command prompt - Add inline prompt with explicit "Use `gh pr comment`" instruction - Add `claude_args` with the gh CLI tool allow-list - Add `allowed_bots: copilot-swe-agent` Trigger types are preserved unchanged (`opened, synchronize, ready_for_review, reopened`) — no functional reason to align them with the library's narrower trigger set, and shrinking them would miss some PR state transitions. After this lands on main, future PRs (and any rebased existing PR) will pick up the new workflow on their next push and produce real Claude review comments instead of the silent-no-op behaviour. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Updates the repository’s Claude Code Review GitHub Actions workflow to reliably post PR review feedback by switching from a plugin-based invocation to an inline prompt that explicitly instructs Claude to post via gh pr comment.
Changes:
- Remove plugin marketplace/plugin configuration and the slash-command prompt.
- Add an inline review prompt that instructs Claude to post results using
gh pr comment. - Add
claude_argsto restrict the allowed Bash tools to a specificghallow-list, and setallowed_bots.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
adnaan
added a commit
that referenced
this pull request
Apr 15, 2026
Seven items from Claude Code Review on the rebased session-3 branch (now that the workflow fix from #71 is on main, Claude is finally posting reviews on examples PRs — this is the first one): 1. LazyLoad goroutines now use the documented cancellation pattern (handlers_loading.go) OnConnect and Reload previously ignored TriggerAction errors via `_ = session.TriggerAction(...)`. ProgressBarController.Start uses the documented `if err := session.TriggerAction(...); err != nil { return }` form to bail when the session disconnects mid-loop. Adopting the same form here makes the cancellation pattern consistent across the file and matches the proposal's Server Push pattern (#31). For the lazy-load case the goroutine is short enough that leaking it is harmless in practice, but consistency matters more. 2. Reconnect-during-loading note in OnConnect Claude observed that if the client reconnects within the 2s loading window, OnConnect fires again and a second goroutine spawns while the first is still asleep. With the err-returning pattern from #1, the first goroutine's TriggerAction returns an error (stale session) and exits cleanly — only the most recent goroutine completes. Added a comment explaining this implicit reliance on framework session-invalidation semantics. 3. Misleading comment on the OnConnect nil-session guard The previous comment claimed "the spinner-forever case is the documented JS-disabled fallback". This was wrong: JS-disabled clients never reach OnConnect at all (no WebSocket connection means OnConnect is never called). The actual JS-disabled fallback is created by Mount() returning Loading=true on the initial HTTP GET. The nil branch in OnConnect is purely a defensive guard against framework regressions — fixed the comment to say so. 4. TestLazyLoading/Reload_Refetches_Fresh_Content trivially-true inequality (patterns_test.go) The previous assertion `if firstContent == secondContent` was trivially false because the two strings have different prefixes ("Content loaded lazily at …" vs "Content reloaded at …") — they could never be equal regardless of timing. Replaced with positive assertions that firstContent contains the initial-load prefix and does NOT contain the reload prefix, and vice versa for secondContent. This actually proves both strings were generated by their respective controller paths (initial OnConnect vs Reload action), which was the original intent. 5. async-operations.tmpl: <mark> → <del> for error block (templates/loading/async-operations.tmpl, patterns_test.go) CLAUDE.md convention is `<del style="display:block;text- decoration:none">` for block-level error messages, while `<mark>` is reserved for highlighted/badge text. Switched the error detail block to <del> for consistency with the rest of the patterns example. Updated TestAsyncOperations selectors at three sites (Initial_Load presence check, Fetch_Transitions wait condition, outcome detection switch) to match. 6. Login regression test: narrow `ins` selector to id (login/login_test.go, login/templates/auth.html) The previous test selector `WaitForText("ins", ...)` matched any <ins> on the page. If the login template ever gains another styled <ins> (e.g., a generic success flash), the test could match the wrong element and pass spuriously. Added an `id="server-welcome-message"` to the auth.html template's ServerMessage <ins> and switched the test to use the explicit id selector. The id is stable and unambiguous. 7. Reload implicit-guard comment Bundled into #1's comment block. ProgressBarController.Start has an explicit `if state.Running { return }` guard against double- click stacking. LazyLoadController.Reload doesn't need one because the template hides the Reload button while Loading=true. Added a short comment explaining the asymmetry so future readers don't wonder why the patterns differ. All Session 3 + login tests still pass against the published livetemplate v0.8.18. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
adnaan
added a commit
that referenced
this pull request
Apr 15, 2026
…ss (#70) * feat: patterns example Session 3 (patterns #14-16) — loading & progress Implements the three Loading & Progress patterns from the patterns example proposal (livetemplate#333): - Pattern #14: Lazy Loading - Pattern #15: Progress Bar - Pattern #16: Async Operations All three rely on `session.TriggerAction()` from a background goroutine to push state updates to the client without polling. None of them worked against the shipping livetemplate v0.8.17 because `ctx.Session()` returned nil — the Session interface was declared but never wired into any production code path. The library gap is fixed in livetemplate#336 (merged as 4883481). ## What's new **state_loading.go** — three state structs (LazyLoadState, ProgressBarState, AsyncOpsState) following the Title/Category-first convention from Sessions 1 and 2. **handlers_loading.go** — three controllers + handler factories: - `LazyLoadController.OnConnect` spawns a 2s goroutine that calls TriggerAction("dataLoaded", ...) once. Reload action re-triggers the same path. Mount() guards on `ctx.Action() == ""` so the Reload POST doesn't reset state. - `ProgressBarController.Start` spawns a goroutine that ticks TriggerAction("updateProgress", {progress: int}) every 500ms for 10 iterations. UpdateProgress reads the int via ctx.GetInt and emits a `success` flash on completion. The `Running` flag prevents double-click stacking within a session. - `AsyncOpsController.Fetch` spawns a goroutine that sleeps 2s and calls TriggerAction("fetchResult", ...) with either success or error data (~33% simulated failure rate via math/rand). FetchResult routes to "success" / "error" via ctx.GetBool and emits the matching flash. **templates/loading/{lazy-loading,progress-bar,async-operations}.tmpl** — Pico CSS markup using `<progress>`, `<blockquote>`, `<mark>`, and the FlashTag helper. The async-operations template uses {{with}} on .Result and .Error to drop the redundant {{if eq .Status}} boilerplate around the FlashTag calls — FlashTag is self-guarding when the flash key isn't set, and {{with}} on the mutually-exclusive Result/Error strings handles the success/error rendering branch without referencing .Status. **main.go** — three new mux.Handle registrations under /patterns/loading/. **data.go** — flips Implemented:true on the three Loading & Progress entries in allPatterns(). The index template iterates allPatterns() data-driven, so no index.tmpl edits are required. **patterns_test.go** — three new test functions: - TestLazyLoading: Initial spinner, data arrives via push, Reload produces fresh content with a different timestamp. - TestProgressBar: Initial state, Start runs to completion (ticks visible, success flash, "Run Again" button), Run Again restarts. - TestAsyncOperations: Initial state, Fetch transitions through loading → success/error (random outcome accepted), matching flash element verified. All tests use condition-based waits (e2etest.WaitForText, WaitFor with real JS predicates) and the shared setupTest fixture. None use chromedp.Sleep. **login/login_test.go** — adds Server_Welcome_Message_via_WebSocket_Push subtest to TestLoginE2E. Before the library fix, the login example's sendWelcomeMessage goroutine silently no-op'd because ctx.Session() returned nil; the existing "Successful Login" test passed only because it asserted on the static "Welcome, testuser!" template literal, not the server-pushed timestamped message. The new subtest waits for "pushed from the server" text in the <ins> element with a 5s timeout — without the library fix the goroutine never fires and this assertion times out. ## Dependencies Requires livetemplate >= v0.8.18 (the next release after the merged fix in livetemplate#336). go.mod will be bumped in a follow-up commit on this branch once the library release lands. Until then, the branch builds against the local main checkout via go.work for manual testing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore(deps): bump livetemplate to v0.8.18 for Session 3 patterns v0.8.18 contains the Session.TriggerAction fix (livetemplate#336) required by Session 3 patterns #14-16 (Lazy Loading, Progress Bar, Async Operations). Previously the library declared the Session interface but never wired ctx.WithSession() into any production code path, so ctx.Session() returned nil and goroutine-based server push silently no-op'd. v0.8.18 wires Session at all five NewContext call sites (WS lifecycle, WS action, HTTP lifecycle, HTTP action, PubSub server-action dispatch) plus the previously-missing handleDispatchedAction and upload completion paths. This bump also activates the regression test for the login example's sendWelcomeMessage server push (added in 4f45ebe), which was previously broken in the same way. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 review comments Six items from Copilot review on the Session 3 patterns PR: 1. LazyLoadController.Reload — check session before mutating state (handlers_loading.go) Copilot: Reload set state.Loading=true and state.Data="" first, then early-returned if ctx.Session() was nil — leaving the page in a permanent loading spinner with no recovery path. Reordered so the session check happens first; if nil, return without touching state. With livetemplate v0.8.18+ the nil branch is unreachable (every action context now has WithSession wired), but the safer ordering means a future framework regression surfaces as "Reload does nothing" rather than "spinner stuck forever". 2. LazyLoadController.OnConnect — clarify nil-session intent (handlers_loading.go) Copilot raised the same "stuck loading forever" concern for OnConnect, but the existing structure was already correct (session check before goroutine spawn). Updated the doc comment to explicitly call out (a) why the defensive check stays even though v0.8.18+ guarantees non-nil session, and (b) that the "spinner-forever-with-JS-disabled" case is the documented fallback behaviour for the Lazy Loading pattern, not a bug. 3. ProgressBarController.Start — check session before Running=true (handlers_loading.go) Copilot: Start set Running=true before checking ctx.Session(). If session were nil the goroutine wouldn't spawn, but Running would stay true forever and the Running guard at the top of Start would block all subsequent button clicks. Reordered to check session first. 4. AsyncOpsController.Fetch — check session before Status="loading" (handlers_loading.go) Same pattern as #3. The button is template-disabled when Status=="loading", so leaving Status pinned to "loading" with no goroutine to clear it would freeze the UI. Reordered. 5. math/rand "non-deterministic by design" comment — clarify (handlers_loading.go) Copilot incorrectly claimed math/rand top-level functions are deterministic unless seeded. They were until Go 1.19, but Go 1.20 changed math/rand to auto-seed at program startup from a system source. Updated the comment to explicitly cite the Go 1.20 behaviour change so the reasoning is clear and the comment doesn't read as wishful thinking. 6. TestAsyncOperations flash assertion strength (patterns_test.go) Copilot: the test only checked for presence of the output[data-flash="success"|"error"] element. An empty placeholder element with no text would satisfy the selector and silently mask a regression where SetFlash wasn't called. Strengthened the assertion to read element.textContent and verify it contains the expected literal flash text ("Fetch complete" or "Fetch failed"), matching what the controller's FetchResult method passes to ctx.SetFlash. Items explicitly NOT changed: - Copilot's suggestion to add synchronous data-load fallbacks for non-WebSocket clients in Lazy Loading. The proposal explicitly documents the JS-disabled limitation as expected behaviour for pattern #14 — adding a synchronous fallback would defeat the pattern's purpose of demonstrating server push, and the spinner- forever case is preferable to silently rendering empty content. - Copilot's suggestion to add "live session unavailable" error flashes for Progress Bar / Async Ops. With v0.8.18+ the case is unreachable, so the speculative recovery UI would just be dead code that confuses future readers. All Session 3 tests still pass against the published v0.8.18. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 round-2 Claude review comments Seven items from Claude Code Review on the rebased session-3 branch (now that the workflow fix from #71 is on main, Claude is finally posting reviews on examples PRs — this is the first one): 1. LazyLoad goroutines now use the documented cancellation pattern (handlers_loading.go) OnConnect and Reload previously ignored TriggerAction errors via `_ = session.TriggerAction(...)`. ProgressBarController.Start uses the documented `if err := session.TriggerAction(...); err != nil { return }` form to bail when the session disconnects mid-loop. Adopting the same form here makes the cancellation pattern consistent across the file and matches the proposal's Server Push pattern (#31). For the lazy-load case the goroutine is short enough that leaking it is harmless in practice, but consistency matters more. 2. Reconnect-during-loading note in OnConnect Claude observed that if the client reconnects within the 2s loading window, OnConnect fires again and a second goroutine spawns while the first is still asleep. With the err-returning pattern from #1, the first goroutine's TriggerAction returns an error (stale session) and exits cleanly — only the most recent goroutine completes. Added a comment explaining this implicit reliance on framework session-invalidation semantics. 3. Misleading comment on the OnConnect nil-session guard The previous comment claimed "the spinner-forever case is the documented JS-disabled fallback". This was wrong: JS-disabled clients never reach OnConnect at all (no WebSocket connection means OnConnect is never called). The actual JS-disabled fallback is created by Mount() returning Loading=true on the initial HTTP GET. The nil branch in OnConnect is purely a defensive guard against framework regressions — fixed the comment to say so. 4. TestLazyLoading/Reload_Refetches_Fresh_Content trivially-true inequality (patterns_test.go) The previous assertion `if firstContent == secondContent` was trivially false because the two strings have different prefixes ("Content loaded lazily at …" vs "Content reloaded at …") — they could never be equal regardless of timing. Replaced with positive assertions that firstContent contains the initial-load prefix and does NOT contain the reload prefix, and vice versa for secondContent. This actually proves both strings were generated by their respective controller paths (initial OnConnect vs Reload action), which was the original intent. 5. async-operations.tmpl: <mark> → <del> for error block (templates/loading/async-operations.tmpl, patterns_test.go) CLAUDE.md convention is `<del style="display:block;text- decoration:none">` for block-level error messages, while `<mark>` is reserved for highlighted/badge text. Switched the error detail block to <del> for consistency with the rest of the patterns example. Updated TestAsyncOperations selectors at three sites (Initial_Load presence check, Fetch_Transitions wait condition, outcome detection switch) to match. 6. Login regression test: narrow `ins` selector to id (login/login_test.go, login/templates/auth.html) The previous test selector `WaitForText("ins", ...)` matched any <ins> on the page. If the login template ever gains another styled <ins> (e.g., a generic success flash), the test could match the wrong element and pass spuriously. Added an `id="server-welcome-message"` to the auth.html template's ServerMessage <ins> and switched the test to use the explicit id selector. The id is stable and unambiguous. 7. Reload implicit-guard comment Bundled into #1's comment block. ProgressBarController.Start has an explicit `if state.Running { return }` guard against double- click stacking. LazyLoadController.Reload doesn't need one because the template hides the Reload button while Loading=true. Added a short comment explaining the asymmetry so future readers don't wonder why the patterns differ. All Session 3 + login tests still pass against the published livetemplate v0.8.18. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * revert: keep <mark> for async-operations error detail The previous commit (e32e703) switched the error block in async-operations.tmpl from <mark> to <del style="display:block; text-decoration:none">{{.}}</del> based on Claude's review nit flagging the deviation from CLAUDE.md convention. Reverting that specific change after follow-up discussion: - <mark> is shorter and reads cleaner in the template - <mark> is the more semantic choice for "highlighted text" which matches the error-detail role: the FlashTag above is the primary error indicator, and <mark> draws attention to the specific error string as a secondary highlight - Pico's default yellow background for <mark> works fine as a visual cue without needing block-level error styling - If a more error-specific look is wanted later, livetemplate.css can override mark[role="error"] or similar — keeping the markup tight today does not preclude a stronger style tomorrow The CLAUDE.md guidance ("<del> for error messages") is preserved unchanged for now since this is one example's deliberate deviation, not a project-wide convention shift. If the team decides to allow <mark> for inline error highlights more broadly, that's a separate docs change. Test selectors at three sites in TestAsyncOperations are reverted back to `mark` to match the template. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 round-3 Claude review comments Six items from Claude's two reviews on the recent commits: 1. AsyncOpsController.Fetch missing concurrent-call guard (handlers_loading.go) Real bug: ProgressBarController.Start guards against re-entry via `if state.Running { return }`, but AsyncOpsController.Fetch had no equivalent. The button is template-disabled during loading, but a direct WebSocket message bypassing the rendered UI could spawn two parallel goroutines, each calling TriggerAction("fetchResult") and producing duplicate state transitions and SetFlash calls. Added `if state.Status == "loading" { return state, nil }` at the top of Fetch, mirroring ProgressBarController's guard. 2. AsyncOpsController goroutine err-return for consistency (handlers_loading.go) The goroutine used `_ = session.TriggerAction(...)` on both branches while LazyLoadController and ProgressBarController use the `if err := session.TriggerAction(...); err != nil { return }` form. This is harmless for AsyncOps because the goroutine is single-shot (one TriggerAction call then exit), but inconsistent with the established pattern. Switched both branches to the err-return form so readers learning from this example see the idiomatic shape everywhere. Added a comment explaining that the err-return is defense-in-depth for a single-shot goroutine. 3. TestProgressBar/Start_Runs_To_Completion intermediate-tick check (patterns_test.go) The previous WaitFor only asserted `progress.value > 0`, which would also be satisfied by a regression where the goroutine instantly jumps to 100 without intermediate ticks. Strengthened to `progress.value > 0 && progress.value < 100`, matching the Run_Again_Restarts_Timer subtest. This proves the goroutine is actually ticking in 10% increments, not jumping straight to done. 4. progress-bar.tmpl: 100% complete label in Done state (templates/loading/progress-bar.tmpl) During Running, the template shows the progress bar plus a `<p><small>{{.Progress}}% complete</small></p>` label. When the job finishes and transitions to Done, the label disappeared, creating a small visual discontinuity (label visible at 10–90%, then vanishes at 100). Added `<p><small>100% complete</small></p>` to the Done branch for visual continuity. 5. aria-live on the error mark for screen reader announcement (templates/loading/async-operations.tmpl) Wrapped the error <mark> in `<p aria-live="assertive">` so screen readers announce the error when the page transitions from loading to error state. The success branch's <blockquote> doesn't need aria-live because the FlashTag above already has appropriate ARIA semantics (output[role="status"] from the FlashTag helper). The error path's FlashTag uses role="alert" but the additional error detail in the mark needed its own announcement signal. 6. Concurrent Fetch regression test (patterns_test.go) Added TestAsyncOperations/Concurrent_Fetch_Reaches_Single_Result. Sends two `fetch` actions in immediate sequence via direct WebSocket message (bypassing the disabled button), waits for the cycle to complete, and asserts exactly one result element (blockquote OR mark) is present. This is a smoke test for the user-visible invariant — concurrent Fetches don't break the page — rather than a direct test of the guard logic, because detecting "the second call was rejected" from the rendered HTML is hard when the state machine is idempotent in its final state. Items NOT addressed: - Test description string "mark element" → already accurate after the previous revert (template uses <mark>, description says "mark"). - Reload button disable while Loading → the framework auto-adds aria-busy on form buttons during the WS round-trip, AND the Reload button only renders in the {{else}} branch when Loading=false, so a separate disabled attribute would be redundant. - TestLazyLoading no JS-disabled path test → not actionable, matches how all other patterns are structured. The JS-disabled spinner- forever case is documented in handlers_loading.go's OnConnect comment block. - async-operations <mark> → <del> swap → user explicitly settled this in the previous revert (keeping <mark> as the more semantic choice for "highlighted error detail" with potential CSS override later in livetemplate.css). - Reconnect goroutine comment polish → the existing comment block already explains the mechanics adequately. All Session 3 + login tests still pass against published v0.8.18. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 round-4 Claude review comments Two items from Claude's latest review: 1. Reload double-goroutine comment was inaccurate (handlers_loading.go) Claude correctly caught that my comment claimed "TriggerAction returns an error when DataLoaded clears Loading and a second Reload starts; the err check below handles it" — but TriggerAction errors only on session disconnect, not on state changes. The actual behaviour is: both goroutines run to completion, both call TriggerAction successfully, and the second simply overwrites state.Data with a newer timestamp. This is harmless for the demo but the explanatory comment was wrong. Rewrote the comment to describe the actual mechanism (concurrent completion, harmless overwrite) and noted the migration path if stricter single-flight semantics are wanted later. 2. Document the deliberate <mark> deviation from CLAUDE.md (templates/loading/async-operations.tmpl) Claude has flagged the <mark> usage as a CLAUDE.md convention violation on three consecutive reviews. The user explicitly settled this in an earlier round: <mark> is the more semantic choice for "highlighted error detail" (the FlashTag above with role="alert" is the primary error indicator; this is a secondary highlight of the error string), and Pico's default styling works without needing block-level error markup. To stop the review- comment loop without changing the underlying decision, added an inline {{/* ... */}} template comment directly above the line explaining (a) it's a deliberate deviation, (b) the semantic reasoning, and (c) the override path via livetemplate.css if a stronger error look becomes desired. Future Claude reviews should see the comment and recognise it as a documented deviation rather than an oversight. Items NOT changed: - map[string]interface{} → map[string]any cosmetic suggestion: the livetemplate library defines the TriggerAction signature using interface{}, so callers naturally use the same syntax for type uniformity. Switching only the example would create a stylistic inconsistency with the framework's own API surface. All Session 3 tests still pass against published v0.8.18. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 round-5 Claude review - Document why ProgressBar/AsyncOps controllers don't need OnConnect: state has no `lvt:"persist"` tags, so reconnects always produce fresh state via cloneStateTyped() rather than restoring stuck Running=true / Status="loading". The "stuck after reconnect" scenario the bot flagged cannot occur in ephemeral mode. LazyLoadController needs OnConnect because the spinner→data swap *is* the pattern; ProgressBar/AsyncOps patterns are about the goroutine push itself, and a mid-run disconnect cleanly ends the demo. - map[string]interface{} → map[string]any throughout handlers_loading.go for consistency with linter expectations and bot review. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 round-6 Claude review - TestProgressBar.Run_Again_Restarts_Timer now asserts the success flash re-appears on the second completion. Catches a regression where the controller forgot to call SetFlash on the re-completion path. - Rewrote LazyLoadController.OnConnect reconnect-during-loading comment to accurately describe both possible outcomes (gap → goroutine errors; reconnected → both goroutines dispatch to new connection, second overwrites Data harmlessly). The previous version claimed framework session-invalidation semantics that don't actually exist — sessions are looked up by groupID, and groupID is stable across reconnects. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 round-6 Claude review - Add Loading re-entrancy guard to LazyLoadController.Reload, symmetric with ProgressBarController.Start and AsyncOpsController.Fetch. The previous comment correctly noted that the template hides the button while loading, but a direct WS message could bypass that — the asymmetry with the other two controllers was a trap for readers pattern-matching from this file. - TestAsyncOperations.Fetch_Transitions_Through_Loading_To_Result now WaitFor's the flash element before reading its text, and properly reports the chromedp.Run error instead of swallowing it. Also switches the JS string to fmt.Sprintf for readability. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 round-7 Claude review - Add <noscript> note to lazy-loading.tmpl spinner branch. JS-disabled clients never establish a WebSocket, so the spinner never resolves; this fallback makes the constraint visible to readers in the rendered UI rather than only in Go comments. The pattern itself is still a patterns-demo trade-off (a production lazy-load would render content server-side first), so the noscript text says "this pattern requires JavaScript" rather than pretending it has a real fallback. - Widen TestProgressBar.Run_Again_Restarts_Timer intermediate-tick timeout from 3s → 5s. The goroutine ticks every 500ms over 5s; on a loaded CI runner the first tick can be delayed past 3s and produce a spurious failure even when the goroutine is working correctly. The outer 'Run Again button appears' assertion already enforces full completion within 10s, so widening the inner check is safe. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: sanction <mark> for secondary inline error details in CLAUDE.md The async-operations.tmpl error string was using <mark> as a "deliberate deviation" from CLAUDE.md, with an inline comment explaining the rationale. This commit promotes that exception into the convention itself: <mark> is now the documented choice for secondary inline error details (a specific error string highlighted alongside a primary FlashTag alert that already carries role="alert"). <del> remains the primary error alert. Also updates the inline template comment to reference the new convention instead of explaining a deviation that no longer exists. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: comment FlashTag placement scope in progress-bar.tmpl Document why FlashTag is rendered only inside the .Done branch (the only render path that emits a "success" flash). The idle and Running branches don't need a FlashTag slot because no controller code path emits a flash while either is the active render state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 round-8 Claude review - Sharpen progress-bar.tmpl FlashTag comment to explain the failure mode if the FlashTag is moved outside the .Done branch (the flash would be consumed during a Running/idle render before Done is reached, and the user would never see it). The previous comment described the design but didn't make the load-bearing constraint obvious to a future maintainer. - Bump TestProgressBar.Start_Runs_To_Completion intermediate-tick timeout from 3s → 5s for consistency with Run_Again_Restarts_Timer and to give loaded CI runners headroom before the goroutine completes the full 5s run. - Shorten CLAUDE.md <mark> guideline from 60 words to ~30 while preserving the rule, the FlashTag pairing, the aria-live note, and the "primary vs secondary" rule of thumb. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 round-9 Claude review Use {{.Progress}} instead of literal "100" in the .Done branch of progress-bar.tmpl. The controller currently sets Progress=100 whenever Done=true, so the rendered output is identical, but binding to .Progress makes the template survive any future controller change to the terminal value (e.g., capping at 99 if a follow-up adds a "still finalising" state) without silently showing a stale 100. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: dedupe progress markup in progress-bar.tmpl Factor the <progress> + percent label out of both the .Running and .Done branches into a single {{if or .Running .Done}} block, and restructure the form/button branches as {{if .Done}}{{else if not .Running}}{{end}}. The rendered output is identical in all three states (idle, running, done), but the template no longer duplicates the <progress value="{{.Progress}}" max="100"> markup across two branches — a future styling change only needs to touch one place. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 round-10 Claude review - Move <noscript> in lazy-loading.tmpl outside the {{if .Loading}} block. The fallback is now unconditional at the top of the article so future template restructuring can't accidentally drop it. JS-disabled clients always see Loading=true so the rendered output is unchanged in practice; this is purely a robustness improvement for future maintenance. - TestAsyncOperations.Fetch_Transitions: now checks the chromedp.Run error when reading `outcome`, and uses a single `flashSelector` variable across the WaitFor and Evaluate so the two reads can't drift. The race window the bot flagged was theoretical (subtests are sequential) but the defensive change is trivially safer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
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.
Why
The
Claude Code Reviewworkflow on this repo had not been posting review comments on any PR — the workflow ran successfully (job returned exit 0) but no comment ever appeared. Investigation on #70 found two contributing problems.Problem 1: Plugin-based prompt silently no-ops
The workflow used a plugin-based prompt invocation:
The
code-review@claude-code-pluginsplugin appears to either suppress its review output entirely or use an internal posting path that does not work with the GitHub App token context the action provides. The job step returns success but no PR comment ever appears.Compare to the livetemplate library workflow, which uses an inline prompt + explicit
gh pr commentinstruction. With that approach, Claude generates the review, then runsgh pr commentitself via its Bash tool (using the GH App installation token that the Claude Code Action injects into the runner's environment). This has been working continuously on the library repo and produced 4 review comments across the recent rounds of livetemplate/livetemplate#336.Problem 2: Workflow validation against default branch blocks in-PR fixes
The Claude Code Action validates the PR's workflow file against the default branch's copy as a security mitigation. Any in-PR change to
claude-code-review.ymlitself fails with:This is a legitimate security boundary — without it, anyone with push access to the repo could open a one-line PR that modifies the workflow to inject
gh repo setor arbitrary commands, then trigger the Claude Code Action to execute them with the GH App's elevated token.The implication is that the fix has to land on main first via this dedicated PR before it takes effect on any other PR. We can't validate the change inside an existing feature PR.
Changes
plugin_marketplaces,plugins, and the slash-commandpromptgh pr comment" instruction (verbatim from the library workflow, withREPO/PR NUMBERinterpolation)claude_argswith allow-list for:gh issue view,gh search,gh issue list,gh pr comment,gh pr diff,gh pr view,gh pr listallowed_bots: copilot-swe-agent(matches library config; lets Claude interact with Copilot comments on the same PR)Trigger types are preserved unchanged (
opened, synchronize, ready_for_review, reopened) — no functional reason to align them with the library's narrower set.After this merges
livetemplate/exampleswill get real Claude review comments within ~5–8 minutes of each pushTest plan
This PR can't really be tested in isolation — the workflow is only triggered by
pull_requestevents, and the onlypull_requestevent for this PR is the PR-itself event. The first real validation will happen on the next PR to land after this merges.🤖 Generated with Claude Code