Skip to content

[codex] Stabilize reviewed state and terminal helper recovery#103

Merged
fgilio merged 10 commits into
mainfrom
codex/reviewed-diff-list-island
Jun 17, 2026
Merged

[codex] Stabilize reviewed state and terminal helper recovery#103
fgilio merged 10 commits into
mainfrom
codex/reviewed-diff-list-island

Conversation

@fgilio

@fgilio fgilio commented Jun 16, 2026

Copy link
Copy Markdown
Owner

Closes: none

Related: none

Summary

Stabilizes the reviewed-file UI path that regressed in the packaged app, refreshes every visible-count surface touched by Hide reviewed, and includes the terminal-helper inbox recovery fixes from the parallel session.

Root Cause

The reviewed controls had three overlapping failure modes:

  • First-party /js/*.js assets were loaded with stable URLs in Electron, so a released app could keep stale Alpine factories after an update.
  • The reviewed-state bridge queued Livewire calls on a remountable Alpine page instance. Livewire island morphs could replace that instance while reviewed actions were in flight, allowing stale snapshots to win.
  • Several visible-count surfaces lived outside the refreshed islands. Hide reviewed updated the sidebar/diff list but could leave status-strip counts or the status-strip copy trigger stale.

The terminal helper also only repaired a stale inbox leaf file. If the app data directory itself was a file, mkdir -p aborted before the helper could write the cold-start inbox path.

Solution

  • Add @localScript / LocalAsset so local JS gets a filemtime cache-busting query string.
  • Re-register Alpine factories on repeated script loads instead of one-shot guarding them.
  • Route reviewed bridge events through tested reviewPage methods and a page-global reviewed-action queue that survives Livewire remounts.
  • Rebuild reviewed state from persisted ReviewedFile rows after toggles so stale Livewire snapshots cannot drop existing marks.
  • Split the duplicate reviewed-summary island into reviewed-toggle and reviewed-counter, then refresh all reviewed and visibility islands explicitly.
  • Island the status-strip copy trigger so it hides when all files are reviewed and hidden.
  • Move reviewed checkbox dispatch inline at the Blade boundary so it does not depend on stale cached JS methods.
  • Harden ./rfa inbox directory creation by moving aside stale non-directory ancestors before creating the path.
  • Refresh the reviewed-state docs and JS loading conventions.

Testing

composer test:lint
composer test:types
npm test
php artisan test --compact tests/Unit/Actions/ToggleReviewedActionTest.php tests/Unit/Livewire/ReviewPageReviewedUndoTest.php tests/Unit/LocalAssetTest.php tests/Arch/BladeConventionsTest.php tests/Unit/Scripts/RfaTerminalHelperTest.php --filter='ToggleReviewedAction|reviewed|local asset|version first-party|terminal helper|hide-reviewed|status-strip copy'
composer test:browser -- --filter='hide reviewed removes|status strip copy menu hides|a second mark in normal mode advances|checking reviewed updates sidebar indicator|checked reviewed checkbox keeps'
for run in {1..20}; do composer test:browser -- --filter='a second mark in normal mode advances' || exit 1; done
php artisan test --compact tests/Arch/SessionRecoveryTest.php
composer test
composer test:browser

Deployment

  • Migrations: none
  • Cache clear: none
  • Monitor: reviewed checkbox glyph, Hide reviewed, status-strip reviewed count, and copy trigger visibility in the packaged app.

fgilio and others added 2 commits June 16, 2026 22:54
Co-authored-by: Codex CLI <fgilio+codex-cli@publica.la>
Co-authored-by: Codex CLI <fgilio+codex-cli@publica.la>
@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b1f673fa-0a66-4b4e-a541-a112823425bb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented Jun 16, 2026

Copy link
Copy Markdown

Screenshots

Updated for 4f1e649.

copy-paths-bulk-default copy-paths-bulk-default
copy-paths-bulk-menu-open copy-paths-bulk-menu-open
copy-paths-single-menu-open copy-paths-single-menu-open

github-actions Bot added a commit that referenced this pull request Jun 16, 2026
fgilio and others added 7 commits June 16, 2026 23:40
…he set

The island refactor moved hideReviewedFiles/showAllFiles and hide-mode
toggleReviewed onto a skipRender() + renderIsland() path. But the
status-strip count band ("N files" / "X/N files") and the sidebar "Files"
header (j/k hint + copy-paths trigger) read visibleFileCount and render
OUTSIDE the three refreshed islands. The previous full render kept them
current; the island path left them showing the pre-hide total while the
sidebar and diff list correctly dropped the reviewed file.

Wrap both regions in their own always:true islands (file-count via a
status-strip slot, file-list-header in the sidebar) and refresh them
through a single renderVisibilityIslands() helper wherever the visible set
changes. Also inline the one-line reviewedChangeNeedsSourceDiffListRender()
wrapper and drop the duplicated skipRender() branch in
settleRecentlyReviewedRender() while consolidating the render glue.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The test hard-coded the macOS "Library/Application Support" inbox base, but
the rfa script branches on `uname`: Linux (the ubuntu-latest CI runner,
where tests/Unit runs in the Core suite) writes to ".local/share". On CI the
script created the XDG dir while the test asserted against the never-touched
Application Support path, so File::isDirectory()/glob assertions failed.

Mirror the script's own branch via PHP_OS_FAMILY so the test targets the
inbox base the helper actually writes to on each platform.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…iewed-state

resources/CLAUDE.md calls these tables canonical ("extend it when adding new
actions"), but the island refactor left them stale: toggleReviewed is now an
unconditional skipRender, hideReviewedFiles/showAllFiles/clearRecentlyReviewed
moved to skipRender + island renders, and the reviewed controls now drive a
window-event bridge (rfa-toggle-reviewed / rfa-hide-reviewed /
rfa-show-all-files / rfa-clear-recently-reviewed) rather than the
toggle-reviewed Livewire event, which is now only reached by unit tests.

Update both tables to match and document why the bridge exists (island-scoped
dispatch).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Locks the visible-file-count regression at the browser level: after hiding a
reviewed file the status-strip band must read "2/3 files" (this assertion
fails against the pre-fix island-only render, which left a stale "3 files"),
and clearing the filter restores it. Uses waitFor so it rides Livewire's
async island morph without racing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Codex CLI <fgilio+codex-cli@publica.la>
Co-authored-by: Codex CLI <fgilio+codex-cli@publica.la>
Co-authored-by: Codex CLI <fgilio+codex-cli@publica.la>
@fgilio fgilio changed the title [codex] Render reviewed state through explicit islands [codex] Stabilize reviewed state and terminal helper recovery Jun 16, 2026
github-actions Bot added a commit that referenced this pull request Jun 16, 2026
The file-header checkbox now inlines its reviewed-toggle logic in the @change handler, leaving createDiffFile.onReviewedChange() with no caller. Remove the orphaned method and its mention in the Alpine-scope contract.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Co-authored-by: Codex CLI <fgilio+codex-cli@publica.la>
github-actions Bot added a commit that referenced this pull request Jun 17, 2026
@fgilio fgilio marked this pull request as ready for review June 17, 2026 09:47
@fgilio fgilio merged commit 5b47df9 into main Jun 17, 2026
15 checks passed
@fgilio fgilio deleted the codex/reviewed-diff-list-island branch June 17, 2026 09:47

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4f1e649bef

ℹ️ 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".

Comment on lines +78 to +81
$hashesByPath = ReviewedFile::query()
->forProjectOrRepo($projectId, $repoPath)
->whereIn('file_path', $paths)
->pluck('content_hash', 'file_path');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep hash validation when rebuilding reviewed state

When a previously reviewed file changes content, SessionStateAction::resolveReviewedFiles() filters that stored row out because its hash no longer matches, but the row remains in reviewed_files. This new reload path plucks every persisted row for the current paths without recomputing or comparing the current hash, so toggling any other file can put the stale path back into $reviewedFiles; ReviewStateService then treats it as reviewed by path alone, allowing Hide reviewed/export state to skip a changed file that was never reviewed at its current content.

Useful? React with 👍 / 👎.

Comment on lines +1001 to +1004
$this->renderIsland('source-diff-list');
$this->renderIsland('file-count');
$this->renderIsland('file-list-header');
$this->renderIsland('status-strip-copy-paths');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep visible-file data in sync with island refreshes

These visibility paths now skip the parent render and refresh only the listed islands, but the Alpine helpers for j/k navigation, isFileVisible(), and the copy-paths menu count read from the page root's data-visible-file-entries, which sits outside these islands and is only regenerated on a parent render. After Hide reviewed/show all or a toggle while hidden, the DOM list updates but that root dataset remains the old list, so keyboard navigation and hidden-file reveal can target files that were just removed.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant