Exclude deleted records from recent history - #39

Open
MACscr wants to merge 1 commit into
awcodes:3.xfrom
MACscr:feature/exclude-deleted-records
Open

Exclude deleted records from recent history#39
MACscr wants to merge 1 commit into
awcodes:3.xfrom
MACscr:feature/exclude-deleted-records

Conversation

@MACscr

Copy link
Copy Markdown

Problem

A RecentEntry stores only a URL, so the menu and the global-search "Recently" group keep listing records that have since been deleted. Clicking one lands the user on Filament's "Record not found" page. Any app using the plugin with deletable (including soft-deletable) records hits this.

Change

  • The recorder already holds the viewed $record and discarded it — it's now stored as a nullable polymorphic recordable reference on the entry. add() gains an optional ?Model $record, plus a small backward-compatible migration.
  • A new existing() scope on RecentEntry limits results to entries whose record still resolves — a MorphTo returns null for a soft-deleted or missing record. Both display surfaces apply it: the menu (RecentlyMenu::getRecords()) and global search (RecentEntryResource::getGlobalSearchEloquentQuery()).
  • Entries with no record reference (e.g. non-record pages) are always kept, so pre-existing rows keep displaying.

Notes

  • Backward compatible / SemVer-safe: additive nullable columns, an optional parameter, and an opt-in scope. No public API broken.
  • Existing installs run the published migration (vendor:publish --tag="recently-migrations" + migrate); README updated with an upgrade note and a "Deleted Records" section.
  • Tests added (tests/src/DeletedRecordsTest.php): a soft-deleted record's entry is hidden from both surfaces while live and reference-less entries remain. composer test is green.

Recent entries store only a URL, so the menu and global search kept
listing records that had since been deleted — clicking one lands on
Filament's "Record not found" page.
Store a nullable polymorphic `recordable` reference when recording an
entry (the recorder already has the record), and add an `existing()`
scope that hides entries whose record no longer resolves, including
soft-deleted ones. Both display surfaces apply the scope. Entries with
no record reference are always kept, so existing rows keep displaying.
@awcodes

Copy link
Copy Markdown
Owner

Can you target this to the 2.x branch?

I think it should have Filament v4 support as well.

I'll merge it to 3.x once everything is green.

@awcodes

Copy link
Copy Markdown
Owner

Also, is there a way to allow the end user to allow for soft deletes if they choose to include them?

@awcodesawcodes left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for this — landing on "Record not found" from the menu is a genuine papercut and storing the record the recorder already has is the right fix. Merges cleanly on 3.x and the suite is green. Two things need addressing before this can go in, though, and they change the release it belongs in.

1. Existing installs break on composer update until they migrate

hasMigrations() in laravel-package-tools only registers migrations for publishingrunsMigrations defaults to false and nothing here calls ->runsMigrations():

// vendor/spatie/laravel-package-tools/src/Concerns/Package/HasMigrations.phppublic bool $runsMigrations = false;

So recordable_type / recordable_id don't exist until each user publishes and runs the new migration. In the meantime add() writes both columns and existing() filters on them, which means viewing any record and rendering the menu both throw SQL errors. The README note is accurate, but it's an instruction people will skip, and the failure mode is a broken panel rather than a degraded one.

Two ways out:

  • Guard the write and the scope on Schema::hasColumn('recent_entries', 'recordable_type'), so an un-migrated install behaves exactly as it does today and picks up filtering once it migrates. That keeps this backward compatible in practice, not just at the API level.
  • Or leave it required and ship it in the next major with the migration as a documented upgrade step.

Happy either way, but as written the "SemVer-safe" note in the description holds for the PHP API and not for users.

2. A stale morph type is a fatal error

orWhereHasMorph('recordable', '*') makes Laravel instantiate every distinct recordable_type found in the table:

// QueriesRelationships::hasMorph()$types = $this->model->newModelQuery()->distinct()->pluck($relation->getMorphType())...
// ...later:$query->where(..., (new$type)->getMorphClass())

Entries are never cleaned up, so as soon as a model is renamed or removed from the app the old rows still hold the old class string:

RecentEntry::create([
'user_id' => $user->id,
'url' => 'https://example.test/gone',
'icon' => '',
'title' => 'Gone',
'recordable_type' => 'App\Models\ThisClassWasDeleted',
'recordable_id' => 1,
]);
RecentEntry::existing()->pluck('url');
Error: Class "App\Models\ThisClassWasDeleted" not found
at vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasRelationships.php:1035
← src/Models/RecentEntry.php:63 (scopeExisting)

That takes down both the menu and global search for the affected user, permanently, until the rows are deleted by hand. Since these entries accumulate forever it's a when-not-if over an app's lifetime. scopeExisting() should resolve the stored types itself and skip any that no longer resolve (Relation::getMorphedModel($type) ?? $type, then class_exists), passing the surviving list to orWhereHasMorph instead of '*'. A test for the stale-type case would be good to have.

Smaller notes

  • The '*' wildcard also runs an unscoped SELECT DISTINCT recordable_type FROM recent_entries — whole table, every user — on each menu render and global search. Indexed by nullableMorphs, so survivable, but it's per-request and resolving the types explicitly (above) lets you cache or narrow it.
  • nullableMorphs respects Schema::defaultMorphKeyType(), so UUID apps are fine if they've set the global default; apps with mixed key types aren't. It's a published migration so users can edit it — worth a line in the README.
  • tests/database/migrations/create_recent_entries_table.php adds nullableMorphs to the create migration, so the actual published add-column migration is never exercised by the suite. Running the real stub would also catch the un-migrated case above.
  • Adding a fourth parameter to Recently::add() breaks any subclass that overrides it, since PHP requires a compatible signature. Unlikely to bite anyone given the facade resolves the concrete class, but flagging it.

If #40 also lands

The two interact: #40 prunes to max_items counting entries whose record is gone, while this PR hides those from display. A user who deletes a lot of records ends up with a near-empty menu while N rows sit in the table. Pruning non-existing entries first would resolve it.

Sign up for freeto 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.

2 participants

@MACscr@awcodes
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Exclude deleted records from recent history - #39

Open
MACscr wants to merge 1 commit into
awcodes:3.xfrom
MACscr:feature/exclude-deleted-records
Open

Exclude deleted records from recent history#39
MACscr wants to merge 1 commit into
awcodes:3.xfrom
MACscr:feature/exclude-deleted-records

Conversation

@MACscr

Copy link
Copy Markdown

Problem

A RecentEntry stores only a URL, so the menu and the global-search "Recently" group keep listing records that have since been deleted. Clicking one lands the user on Filament's "Record not found" page. Any app using the plugin with deletable (including soft-deletable) records hits this.

Change

  • The recorder already holds the viewed $record and discarded it — it's now stored as a nullable polymorphic recordable reference on the entry. add() gains an optional ?Model $record, plus a small backward-compatible migration.
  • A new existing() scope on RecentEntry limits results to entries whose record still resolves — a MorphTo returns null for a soft-deleted or missing record. Both display surfaces apply it: the menu (RecentlyMenu::getRecords()) and global search (RecentEntryResource::getGlobalSearchEloquentQuery()).
  • Entries with no record reference (e.g. non-record pages) are always kept, so pre-existing rows keep displaying.

Notes

  • Backward compatible / SemVer-safe: additive nullable columns, an optional parameter, and an opt-in scope. No public API broken.
  • Existing installs run the published migration (vendor:publish --tag="recently-migrations" + migrate); README updated with an upgrade note and a "Deleted Records" section.
  • Tests added (tests/src/DeletedRecordsTest.php): a soft-deleted record's entry is hidden from both surfaces while live and reference-less entries remain. composer test is green.

Recent entries store only a URL, so the menu and global search kept
listing records that had since been deleted — clicking one lands on
Filament's "Record not found" page.
Store a nullable polymorphic `recordable` reference when recording an
entry (the recorder already has the record), and add an `existing()`
scope that hides entries whose record no longer resolves, including
soft-deleted ones. Both display surfaces apply the scope. Entries with
no record reference are always kept, so existing rows keep displaying.
@awcodes

Copy link
Copy Markdown
Owner

Can you target this to the 2.x branch?

I think it should have Filament v4 support as well.

I'll merge it to 3.x once everything is green.

@awcodes

Copy link
Copy Markdown
Owner

Also, is there a way to allow the end user to allow for soft deletes if they choose to include them?

@awcodesawcodes left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for this — landing on "Record not found" from the menu is a genuine papercut and storing the record the recorder already has is the right fix. Merges cleanly on 3.x and the suite is green. Two things need addressing before this can go in, though, and they change the release it belongs in.

1. Existing installs break on composer update until they migrate

hasMigrations() in laravel-package-tools only registers migrations for publishingrunsMigrations defaults to false and nothing here calls ->runsMigrations():

// vendor/spatie/laravel-package-tools/src/Concerns/Package/HasMigrations.phppublic bool $runsMigrations = false;

So recordable_type / recordable_id don't exist until each user publishes and runs the new migration. In the meantime add() writes both columns and existing() filters on them, which means viewing any record and rendering the menu both throw SQL errors. The README note is accurate, but it's an instruction people will skip, and the failure mode is a broken panel rather than a degraded one.

Two ways out:

  • Guard the write and the scope on Schema::hasColumn('recent_entries', 'recordable_type'), so an un-migrated install behaves exactly as it does today and picks up filtering once it migrates. That keeps this backward compatible in practice, not just at the API level.
  • Or leave it required and ship it in the next major with the migration as a documented upgrade step.

Happy either way, but as written the "SemVer-safe" note in the description holds for the PHP API and not for users.

2. A stale morph type is a fatal error

orWhereHasMorph('recordable', '*') makes Laravel instantiate every distinct recordable_type found in the table:

// QueriesRelationships::hasMorph()$types = $this->model->newModelQuery()->distinct()->pluck($relation->getMorphType())...
// ...later:$query->where(..., (new$type)->getMorphClass())

Entries are never cleaned up, so as soon as a model is renamed or removed from the app the old rows still hold the old class string:

RecentEntry::create([
'user_id' => $user->id,
'url' => 'https://example.test/gone',
'icon' => '',
'title' => 'Gone',
'recordable_type' => 'App\Models\ThisClassWasDeleted',
'recordable_id' => 1,
]);
RecentEntry::existing()->pluck('url');
Error: Class "App\Models\ThisClassWasDeleted" not found
at vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasRelationships.php:1035
← src/Models/RecentEntry.php:63 (scopeExisting)

That takes down both the menu and global search for the affected user, permanently, until the rows are deleted by hand. Since these entries accumulate forever it's a when-not-if over an app's lifetime. scopeExisting() should resolve the stored types itself and skip any that no longer resolve (Relation::getMorphedModel($type) ?? $type, then class_exists), passing the surviving list to orWhereHasMorph instead of '*'. A test for the stale-type case would be good to have.

Smaller notes

  • The '*' wildcard also runs an unscoped SELECT DISTINCT recordable_type FROM recent_entries — whole table, every user — on each menu render and global search. Indexed by nullableMorphs, so survivable, but it's per-request and resolving the types explicitly (above) lets you cache or narrow it.
  • nullableMorphs respects Schema::defaultMorphKeyType(), so UUID apps are fine if they've set the global default; apps with mixed key types aren't. It's a published migration so users can edit it — worth a line in the README.
  • tests/database/migrations/create_recent_entries_table.php adds nullableMorphs to the create migration, so the actual published add-column migration is never exercised by the suite. Running the real stub would also catch the un-migrated case above.
  • Adding a fourth parameter to Recently::add() breaks any subclass that overrides it, since PHP requires a compatible signature. Unlikely to bite anyone given the facade resolves the concrete class, but flagging it.

If #40 also lands

The two interact: #40 prunes to max_items counting entries whose record is gone, while this PR hides those from display. A user who deletes a lot of records ends up with a near-empty menu while N rows sit in the table. Pruning non-existing entries first would resolve it.

Sign up for freeto 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.

2 participants

@MACscr@awcodes
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Exclude deleted records from recent history - #39

Open
MACscr wants to merge 1 commit into
awcodes:3.xfrom
MACscr:feature/exclude-deleted-records
Open

Exclude deleted records from recent history#39
MACscr wants to merge 1 commit into
awcodes:3.xfrom
MACscr:feature/exclude-deleted-records

Conversation

@MACscr

Copy link
Copy Markdown

Problem

A RecentEntry stores only a URL, so the menu and the global-search "Recently" group keep listing records that have since been deleted. Clicking one lands the user on Filament's "Record not found" page. Any app using the plugin with deletable (including soft-deletable) records hits this.

Change

  • The recorder already holds the viewed $record and discarded it — it's now stored as a nullable polymorphic recordable reference on the entry. add() gains an optional ?Model $record, plus a small backward-compatible migration.
  • A new existing() scope on RecentEntry limits results to entries whose record still resolves — a MorphTo returns null for a soft-deleted or missing record. Both display surfaces apply it: the menu (RecentlyMenu::getRecords()) and global search (RecentEntryResource::getGlobalSearchEloquentQuery()).
  • Entries with no record reference (e.g. non-record pages) are always kept, so pre-existing rows keep displaying.

Notes

  • Backward compatible / SemVer-safe: additive nullable columns, an optional parameter, and an opt-in scope. No public API broken.
  • Existing installs run the published migration (vendor:publish --tag="recently-migrations" + migrate); README updated with an upgrade note and a "Deleted Records" section.
  • Tests added (tests/src/DeletedRecordsTest.php): a soft-deleted record's entry is hidden from both surfaces while live and reference-less entries remain. composer test is green.

Recent entries store only a URL, so the menu and global search kept
listing records that had since been deleted — clicking one lands on
Filament's "Record not found" page.
Store a nullable polymorphic `recordable` reference when recording an
entry (the recorder already has the record), and add an `existing()`
scope that hides entries whose record no longer resolves, including
soft-deleted ones. Both display surfaces apply the scope. Entries with
no record reference are always kept, so existing rows keep displaying.
@awcodes

Copy link
Copy Markdown
Owner

Can you target this to the 2.x branch?

I think it should have Filament v4 support as well.

I'll merge it to 3.x once everything is green.

@awcodes

Copy link
Copy Markdown
Owner

Also, is there a way to allow the end user to allow for soft deletes if they choose to include them?

@awcodesawcodes left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for this — landing on "Record not found" from the menu is a genuine papercut and storing the record the recorder already has is the right fix. Merges cleanly on 3.x and the suite is green. Two things need addressing before this can go in, though, and they change the release it belongs in.

1. Existing installs break on composer update until they migrate

hasMigrations() in laravel-package-tools only registers migrations for publishingrunsMigrations defaults to false and nothing here calls ->runsMigrations():

// vendor/spatie/laravel-package-tools/src/Concerns/Package/HasMigrations.phppublic bool $runsMigrations = false;

So recordable_type / recordable_id don't exist until each user publishes and runs the new migration. In the meantime add() writes both columns and existing() filters on them, which means viewing any record and rendering the menu both throw SQL errors. The README note is accurate, but it's an instruction people will skip, and the failure mode is a broken panel rather than a degraded one.

Two ways out:

  • Guard the write and the scope on Schema::hasColumn('recent_entries', 'recordable_type'), so an un-migrated install behaves exactly as it does today and picks up filtering once it migrates. That keeps this backward compatible in practice, not just at the API level.
  • Or leave it required and ship it in the next major with the migration as a documented upgrade step.

Happy either way, but as written the "SemVer-safe" note in the description holds for the PHP API and not for users.

2. A stale morph type is a fatal error

orWhereHasMorph('recordable', '*') makes Laravel instantiate every distinct recordable_type found in the table:

// QueriesRelationships::hasMorph()$types = $this->model->newModelQuery()->distinct()->pluck($relation->getMorphType())...
// ...later:$query->where(..., (new$type)->getMorphClass())

Entries are never cleaned up, so as soon as a model is renamed or removed from the app the old rows still hold the old class string:

RecentEntry::create([
'user_id' => $user->id,
'url' => 'https://example.test/gone',
'icon' => '',
'title' => 'Gone',
'recordable_type' => 'App\Models\ThisClassWasDeleted',
'recordable_id' => 1,
]);
RecentEntry::existing()->pluck('url');
Error: Class "App\Models\ThisClassWasDeleted" not found
at vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasRelationships.php:1035
← src/Models/RecentEntry.php:63 (scopeExisting)

That takes down both the menu and global search for the affected user, permanently, until the rows are deleted by hand. Since these entries accumulate forever it's a when-not-if over an app's lifetime. scopeExisting() should resolve the stored types itself and skip any that no longer resolve (Relation::getMorphedModel($type) ?? $type, then class_exists), passing the surviving list to orWhereHasMorph instead of '*'. A test for the stale-type case would be good to have.

Smaller notes

  • The '*' wildcard also runs an unscoped SELECT DISTINCT recordable_type FROM recent_entries — whole table, every user — on each menu render and global search. Indexed by nullableMorphs, so survivable, but it's per-request and resolving the types explicitly (above) lets you cache or narrow it.
  • nullableMorphs respects Schema::defaultMorphKeyType(), so UUID apps are fine if they've set the global default; apps with mixed key types aren't. It's a published migration so users can edit it — worth a line in the README.
  • tests/database/migrations/create_recent_entries_table.php adds nullableMorphs to the create migration, so the actual published add-column migration is never exercised by the suite. Running the real stub would also catch the un-migrated case above.
  • Adding a fourth parameter to Recently::add() breaks any subclass that overrides it, since PHP requires a compatible signature. Unlikely to bite anyone given the facade resolves the concrete class, but flagging it.

If #40 also lands

The two interact: #40 prunes to max_items counting entries whose record is gone, while this PR hides those from display. A user who deletes a lot of records ends up with a near-empty menu while N rows sit in the table. Pruning non-existing entries first would resolve it.

Sign up for freeto 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.

2 participants

@MACscr@awcodes
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Exclude deleted records from recent history - #39

Open
MACscr wants to merge 1 commit into
awcodes:3.xfrom
MACscr:feature/exclude-deleted-records
Open

Exclude deleted records from recent history#39
MACscr wants to merge 1 commit into
awcodes:3.xfrom
MACscr:feature/exclude-deleted-records

Conversation

@MACscr

Copy link
Copy Markdown

Problem

A RecentEntry stores only a URL, so the menu and the global-search "Recently" group keep listing records that have since been deleted. Clicking one lands the user on Filament's "Record not found" page. Any app using the plugin with deletable (including soft-deletable) records hits this.

Change

  • The recorder already holds the viewed $record and discarded it — it's now stored as a nullable polymorphic recordable reference on the entry. add() gains an optional ?Model $record, plus a small backward-compatible migration.
  • A new existing() scope on RecentEntry limits results to entries whose record still resolves — a MorphTo returns null for a soft-deleted or missing record. Both display surfaces apply it: the menu (RecentlyMenu::getRecords()) and global search (RecentEntryResource::getGlobalSearchEloquentQuery()).
  • Entries with no record reference (e.g. non-record pages) are always kept, so pre-existing rows keep displaying.

Notes

  • Backward compatible / SemVer-safe: additive nullable columns, an optional parameter, and an opt-in scope. No public API broken.
  • Existing installs run the published migration (vendor:publish --tag="recently-migrations" + migrate); README updated with an upgrade note and a "Deleted Records" section.
  • Tests added (tests/src/DeletedRecordsTest.php): a soft-deleted record's entry is hidden from both surfaces while live and reference-less entries remain. composer test is green.

Recent entries store only a URL, so the menu and global search kept
listing records that had since been deleted — clicking one lands on
Filament's "Record not found" page.
Store a nullable polymorphic `recordable` reference when recording an
entry (the recorder already has the record), and add an `existing()`
scope that hides entries whose record no longer resolves, including
soft-deleted ones. Both display surfaces apply the scope. Entries with
no record reference are always kept, so existing rows keep displaying.
@awcodes

Copy link
Copy Markdown
Owner

Can you target this to the 2.x branch?

I think it should have Filament v4 support as well.

I'll merge it to 3.x once everything is green.

@awcodes

Copy link
Copy Markdown
Owner

Also, is there a way to allow the end user to allow for soft deletes if they choose to include them?

@awcodesawcodes left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for this — landing on "Record not found" from the menu is a genuine papercut and storing the record the recorder already has is the right fix. Merges cleanly on 3.x and the suite is green. Two things need addressing before this can go in, though, and they change the release it belongs in.

1. Existing installs break on composer update until they migrate

hasMigrations() in laravel-package-tools only registers migrations for publishingrunsMigrations defaults to false and nothing here calls ->runsMigrations():

// vendor/spatie/laravel-package-tools/src/Concerns/Package/HasMigrations.phppublic bool $runsMigrations = false;

So recordable_type / recordable_id don't exist until each user publishes and runs the new migration. In the meantime add() writes both columns and existing() filters on them, which means viewing any record and rendering the menu both throw SQL errors. The README note is accurate, but it's an instruction people will skip, and the failure mode is a broken panel rather than a degraded one.

Two ways out:

  • Guard the write and the scope on Schema::hasColumn('recent_entries', 'recordable_type'), so an un-migrated install behaves exactly as it does today and picks up filtering once it migrates. That keeps this backward compatible in practice, not just at the API level.
  • Or leave it required and ship it in the next major with the migration as a documented upgrade step.

Happy either way, but as written the "SemVer-safe" note in the description holds for the PHP API and not for users.

2. A stale morph type is a fatal error

orWhereHasMorph('recordable', '*') makes Laravel instantiate every distinct recordable_type found in the table:

// QueriesRelationships::hasMorph()$types = $this->model->newModelQuery()->distinct()->pluck($relation->getMorphType())...
// ...later:$query->where(..., (new$type)->getMorphClass())

Entries are never cleaned up, so as soon as a model is renamed or removed from the app the old rows still hold the old class string:

RecentEntry::create([
'user_id' => $user->id,
'url' => 'https://example.test/gone',
'icon' => '',
'title' => 'Gone',
'recordable_type' => 'App\Models\ThisClassWasDeleted',
'recordable_id' => 1,
]);
RecentEntry::existing()->pluck('url');
Error: Class "App\Models\ThisClassWasDeleted" not found
at vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasRelationships.php:1035
← src/Models/RecentEntry.php:63 (scopeExisting)

That takes down both the menu and global search for the affected user, permanently, until the rows are deleted by hand. Since these entries accumulate forever it's a when-not-if over an app's lifetime. scopeExisting() should resolve the stored types itself and skip any that no longer resolve (Relation::getMorphedModel($type) ?? $type, then class_exists), passing the surviving list to orWhereHasMorph instead of '*'. A test for the stale-type case would be good to have.

Smaller notes

  • The '*' wildcard also runs an unscoped SELECT DISTINCT recordable_type FROM recent_entries — whole table, every user — on each menu render and global search. Indexed by nullableMorphs, so survivable, but it's per-request and resolving the types explicitly (above) lets you cache or narrow it.
  • nullableMorphs respects Schema::defaultMorphKeyType(), so UUID apps are fine if they've set the global default; apps with mixed key types aren't. It's a published migration so users can edit it — worth a line in the README.
  • tests/database/migrations/create_recent_entries_table.php adds nullableMorphs to the create migration, so the actual published add-column migration is never exercised by the suite. Running the real stub would also catch the un-migrated case above.
  • Adding a fourth parameter to Recently::add() breaks any subclass that overrides it, since PHP requires a compatible signature. Unlikely to bite anyone given the facade resolves the concrete class, but flagging it.

If #40 also lands

The two interact: #40 prunes to max_items counting entries whose record is gone, while this PR hides those from display. A user who deletes a lot of records ends up with a near-empty menu while N rows sit in the table. Pruning non-existing entries first would resolve it.

Sign up for freeto 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.

2 participants

@MACscr@awcodes
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Exclude deleted records from recent history - #39

Open
MACscr wants to merge 1 commit into
awcodes:3.xfrom
MACscr:feature/exclude-deleted-records
Open

Exclude deleted records from recent history#39
MACscr wants to merge 1 commit into
awcodes:3.xfrom
MACscr:feature/exclude-deleted-records

Conversation

@MACscr

Copy link
Copy Markdown

Problem

A RecentEntry stores only a URL, so the menu and the global-search "Recently" group keep listing records that have since been deleted. Clicking one lands the user on Filament's "Record not found" page. Any app using the plugin with deletable (including soft-deletable) records hits this.

Change

  • The recorder already holds the viewed $record and discarded it — it's now stored as a nullable polymorphic recordable reference on the entry. add() gains an optional ?Model $record, plus a small backward-compatible migration.
  • A new existing() scope on RecentEntry limits results to entries whose record still resolves — a MorphTo returns null for a soft-deleted or missing record. Both display surfaces apply it: the menu (RecentlyMenu::getRecords()) and global search (RecentEntryResource::getGlobalSearchEloquentQuery()).
  • Entries with no record reference (e.g. non-record pages) are always kept, so pre-existing rows keep displaying.

Notes

  • Backward compatible / SemVer-safe: additive nullable columns, an optional parameter, and an opt-in scope. No public API broken.
  • Existing installs run the published migration (vendor:publish --tag="recently-migrations" + migrate); README updated with an upgrade note and a "Deleted Records" section.
  • Tests added (tests/src/DeletedRecordsTest.php): a soft-deleted record's entry is hidden from both surfaces while live and reference-less entries remain. composer test is green.

Recent entries store only a URL, so the menu and global search kept
listing records that had since been deleted — clicking one lands on
Filament's "Record not found" page.
Store a nullable polymorphic `recordable` reference when recording an
entry (the recorder already has the record), and add an `existing()`
scope that hides entries whose record no longer resolves, including
soft-deleted ones. Both display surfaces apply the scope. Entries with
no record reference are always kept, so existing rows keep displaying.
@awcodes

Copy link
Copy Markdown
Owner

Can you target this to the 2.x branch?

I think it should have Filament v4 support as well.

I'll merge it to 3.x once everything is green.

@awcodes

Copy link
Copy Markdown
Owner

Also, is there a way to allow the end user to allow for soft deletes if they choose to include them?

@awcodesawcodes left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for this — landing on "Record not found" from the menu is a genuine papercut and storing the record the recorder already has is the right fix. Merges cleanly on 3.x and the suite is green. Two things need addressing before this can go in, though, and they change the release it belongs in.

1. Existing installs break on composer update until they migrate

hasMigrations() in laravel-package-tools only registers migrations for publishingrunsMigrations defaults to false and nothing here calls ->runsMigrations():

// vendor/spatie/laravel-package-tools/src/Concerns/Package/HasMigrations.phppublic bool $runsMigrations = false;

So recordable_type / recordable_id don't exist until each user publishes and runs the new migration. In the meantime add() writes both columns and existing() filters on them, which means viewing any record and rendering the menu both throw SQL errors. The README note is accurate, but it's an instruction people will skip, and the failure mode is a broken panel rather than a degraded one.

Two ways out:

  • Guard the write and the scope on Schema::hasColumn('recent_entries', 'recordable_type'), so an un-migrated install behaves exactly as it does today and picks up filtering once it migrates. That keeps this backward compatible in practice, not just at the API level.
  • Or leave it required and ship it in the next major with the migration as a documented upgrade step.

Happy either way, but as written the "SemVer-safe" note in the description holds for the PHP API and not for users.

2. A stale morph type is a fatal error

orWhereHasMorph('recordable', '*') makes Laravel instantiate every distinct recordable_type found in the table:

// QueriesRelationships::hasMorph()$types = $this->model->newModelQuery()->distinct()->pluck($relation->getMorphType())...
// ...later:$query->where(..., (new$type)->getMorphClass())

Entries are never cleaned up, so as soon as a model is renamed or removed from the app the old rows still hold the old class string:

RecentEntry::create([
'user_id' => $user->id,
'url' => 'https://example.test/gone',
'icon' => '',
'title' => 'Gone',
'recordable_type' => 'App\Models\ThisClassWasDeleted',
'recordable_id' => 1,
]);
RecentEntry::existing()->pluck('url');
Error: Class "App\Models\ThisClassWasDeleted" not found
at vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasRelationships.php:1035
← src/Models/RecentEntry.php:63 (scopeExisting)

That takes down both the menu and global search for the affected user, permanently, until the rows are deleted by hand. Since these entries accumulate forever it's a when-not-if over an app's lifetime. scopeExisting() should resolve the stored types itself and skip any that no longer resolve (Relation::getMorphedModel($type) ?? $type, then class_exists), passing the surviving list to orWhereHasMorph instead of '*'. A test for the stale-type case would be good to have.

Smaller notes

  • The '*' wildcard also runs an unscoped SELECT DISTINCT recordable_type FROM recent_entries — whole table, every user — on each menu render and global search. Indexed by nullableMorphs, so survivable, but it's per-request and resolving the types explicitly (above) lets you cache or narrow it.
  • nullableMorphs respects Schema::defaultMorphKeyType(), so UUID apps are fine if they've set the global default; apps with mixed key types aren't. It's a published migration so users can edit it — worth a line in the README.
  • tests/database/migrations/create_recent_entries_table.php adds nullableMorphs to the create migration, so the actual published add-column migration is never exercised by the suite. Running the real stub would also catch the un-migrated case above.
  • Adding a fourth parameter to Recently::add() breaks any subclass that overrides it, since PHP requires a compatible signature. Unlikely to bite anyone given the facade resolves the concrete class, but flagging it.

If #40 also lands

The two interact: #40 prunes to max_items counting entries whose record is gone, while this PR hides those from display. A user who deletes a lot of records ends up with a near-empty menu while N rows sit in the table. Pruning non-existing entries first would resolve it.

Sign up for freeto 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.

2 participants

@MACscr@awcodes
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Exclude deleted records from recent history - #39

Open
MACscr wants to merge 1 commit into
awcodes:3.xfrom
MACscr:feature/exclude-deleted-records
Open

Exclude deleted records from recent history#39
MACscr wants to merge 1 commit into
awcodes:3.xfrom
MACscr:feature/exclude-deleted-records

Conversation

@MACscr

Copy link
Copy Markdown

Problem

A RecentEntry stores only a URL, so the menu and the global-search "Recently" group keep listing records that have since been deleted. Clicking one lands the user on Filament's "Record not found" page. Any app using the plugin with deletable (including soft-deletable) records hits this.

Change

  • The recorder already holds the viewed $record and discarded it — it's now stored as a nullable polymorphic recordable reference on the entry. add() gains an optional ?Model $record, plus a small backward-compatible migration.
  • A new existing() scope on RecentEntry limits results to entries whose record still resolves — a MorphTo returns null for a soft-deleted or missing record. Both display surfaces apply it: the menu (RecentlyMenu::getRecords()) and global search (RecentEntryResource::getGlobalSearchEloquentQuery()).
  • Entries with no record reference (e.g. non-record pages) are always kept, so pre-existing rows keep displaying.

Notes

  • Backward compatible / SemVer-safe: additive nullable columns, an optional parameter, and an opt-in scope. No public API broken.
  • Existing installs run the published migration (vendor:publish --tag="recently-migrations" + migrate); README updated with an upgrade note and a "Deleted Records" section.
  • Tests added (tests/src/DeletedRecordsTest.php): a soft-deleted record's entry is hidden from both surfaces while live and reference-less entries remain. composer test is green.

Recent entries store only a URL, so the menu and global search kept
listing records that had since been deleted — clicking one lands on
Filament's "Record not found" page.
Store a nullable polymorphic `recordable` reference when recording an
entry (the recorder already has the record), and add an `existing()`
scope that hides entries whose record no longer resolves, including
soft-deleted ones. Both display surfaces apply the scope. Entries with
no record reference are always kept, so existing rows keep displaying.
@awcodes

Copy link
Copy Markdown
Owner

Can you target this to the 2.x branch?

I think it should have Filament v4 support as well.

I'll merge it to 3.x once everything is green.

@awcodes

Copy link
Copy Markdown
Owner

Also, is there a way to allow the end user to allow for soft deletes if they choose to include them?

@awcodesawcodes left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for this — landing on "Record not found" from the menu is a genuine papercut and storing the record the recorder already has is the right fix. Merges cleanly on 3.x and the suite is green. Two things need addressing before this can go in, though, and they change the release it belongs in.

1. Existing installs break on composer update until they migrate

hasMigrations() in laravel-package-tools only registers migrations for publishingrunsMigrations defaults to false and nothing here calls ->runsMigrations():

// vendor/spatie/laravel-package-tools/src/Concerns/Package/HasMigrations.phppublic bool $runsMigrations = false;

So recordable_type / recordable_id don't exist until each user publishes and runs the new migration. In the meantime add() writes both columns and existing() filters on them, which means viewing any record and rendering the menu both throw SQL errors. The README note is accurate, but it's an instruction people will skip, and the failure mode is a broken panel rather than a degraded one.

Two ways out:

  • Guard the write and the scope on Schema::hasColumn('recent_entries', 'recordable_type'), so an un-migrated install behaves exactly as it does today and picks up filtering once it migrates. That keeps this backward compatible in practice, not just at the API level.
  • Or leave it required and ship it in the next major with the migration as a documented upgrade step.

Happy either way, but as written the "SemVer-safe" note in the description holds for the PHP API and not for users.

2. A stale morph type is a fatal error

orWhereHasMorph('recordable', '*') makes Laravel instantiate every distinct recordable_type found in the table:

// QueriesRelationships::hasMorph()$types = $this->model->newModelQuery()->distinct()->pluck($relation->getMorphType())...
// ...later:$query->where(..., (new$type)->getMorphClass())

Entries are never cleaned up, so as soon as a model is renamed or removed from the app the old rows still hold the old class string:

RecentEntry::create([
'user_id' => $user->id,
'url' => 'https://example.test/gone',
'icon' => '',
'title' => 'Gone',
'recordable_type' => 'App\Models\ThisClassWasDeleted',
'recordable_id' => 1,
]);
RecentEntry::existing()->pluck('url');
Error: Class "App\Models\ThisClassWasDeleted" not found
at vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasRelationships.php:1035
← src/Models/RecentEntry.php:63 (scopeExisting)

That takes down both the menu and global search for the affected user, permanently, until the rows are deleted by hand. Since these entries accumulate forever it's a when-not-if over an app's lifetime. scopeExisting() should resolve the stored types itself and skip any that no longer resolve (Relation::getMorphedModel($type) ?? $type, then class_exists), passing the surviving list to orWhereHasMorph instead of '*'. A test for the stale-type case would be good to have.

Smaller notes

  • The '*' wildcard also runs an unscoped SELECT DISTINCT recordable_type FROM recent_entries — whole table, every user — on each menu render and global search. Indexed by nullableMorphs, so survivable, but it's per-request and resolving the types explicitly (above) lets you cache or narrow it.
  • nullableMorphs respects Schema::defaultMorphKeyType(), so UUID apps are fine if they've set the global default; apps with mixed key types aren't. It's a published migration so users can edit it — worth a line in the README.
  • tests/database/migrations/create_recent_entries_table.php adds nullableMorphs to the create migration, so the actual published add-column migration is never exercised by the suite. Running the real stub would also catch the un-migrated case above.
  • Adding a fourth parameter to Recently::add() breaks any subclass that overrides it, since PHP requires a compatible signature. Unlikely to bite anyone given the facade resolves the concrete class, but flagging it.

If #40 also lands

The two interact: #40 prunes to max_items counting entries whose record is gone, while this PR hides those from display. A user who deletes a lot of records ends up with a near-empty menu while N rows sit in the table. Pruning non-existing entries first would resolve it.

Sign up for freeto 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.

2 participants

@MACscr@awcodes
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Exclude deleted records from recent history - #39

Open
MACscr wants to merge 1 commit into
awcodes:3.xfrom
MACscr:feature/exclude-deleted-records
Open

Exclude deleted records from recent history#39
MACscr wants to merge 1 commit into
awcodes:3.xfrom
MACscr:feature/exclude-deleted-records

Conversation

@MACscr

Copy link
Copy Markdown

Problem

A RecentEntry stores only a URL, so the menu and the global-search "Recently" group keep listing records that have since been deleted. Clicking one lands the user on Filament's "Record not found" page. Any app using the plugin with deletable (including soft-deletable) records hits this.

Change

  • The recorder already holds the viewed $record and discarded it — it's now stored as a nullable polymorphic recordable reference on the entry. add() gains an optional ?Model $record, plus a small backward-compatible migration.
  • A new existing() scope on RecentEntry limits results to entries whose record still resolves — a MorphTo returns null for a soft-deleted or missing record. Both display surfaces apply it: the menu (RecentlyMenu::getRecords()) and global search (RecentEntryResource::getGlobalSearchEloquentQuery()).
  • Entries with no record reference (e.g. non-record pages) are always kept, so pre-existing rows keep displaying.

Notes

  • Backward compatible / SemVer-safe: additive nullable columns, an optional parameter, and an opt-in scope. No public API broken.
  • Existing installs run the published migration (vendor:publish --tag="recently-migrations" + migrate); README updated with an upgrade note and a "Deleted Records" section.
  • Tests added (tests/src/DeletedRecordsTest.php): a soft-deleted record's entry is hidden from both surfaces while live and reference-less entries remain. composer test is green.

Recent entries store only a URL, so the menu and global search kept
listing records that had since been deleted — clicking one lands on
Filament's "Record not found" page.
Store a nullable polymorphic `recordable` reference when recording an
entry (the recorder already has the record), and add an `existing()`
scope that hides entries whose record no longer resolves, including
soft-deleted ones. Both display surfaces apply the scope. Entries with
no record reference are always kept, so existing rows keep displaying.
@awcodes

Copy link
Copy Markdown
Owner

Can you target this to the 2.x branch?

I think it should have Filament v4 support as well.

I'll merge it to 3.x once everything is green.

@awcodes

Copy link
Copy Markdown
Owner

Also, is there a way to allow the end user to allow for soft deletes if they choose to include them?

@awcodesawcodes left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for this — landing on "Record not found" from the menu is a genuine papercut and storing the record the recorder already has is the right fix. Merges cleanly on 3.x and the suite is green. Two things need addressing before this can go in, though, and they change the release it belongs in.

1. Existing installs break on composer update until they migrate

hasMigrations() in laravel-package-tools only registers migrations for publishingrunsMigrations defaults to false and nothing here calls ->runsMigrations():

// vendor/spatie/laravel-package-tools/src/Concerns/Package/HasMigrations.phppublic bool $runsMigrations = false;

So recordable_type / recordable_id don't exist until each user publishes and runs the new migration. In the meantime add() writes both columns and existing() filters on them, which means viewing any record and rendering the menu both throw SQL errors. The README note is accurate, but it's an instruction people will skip, and the failure mode is a broken panel rather than a degraded one.

Two ways out:

  • Guard the write and the scope on Schema::hasColumn('recent_entries', 'recordable_type'), so an un-migrated install behaves exactly as it does today and picks up filtering once it migrates. That keeps this backward compatible in practice, not just at the API level.
  • Or leave it required and ship it in the next major with the migration as a documented upgrade step.

Happy either way, but as written the "SemVer-safe" note in the description holds for the PHP API and not for users.

2. A stale morph type is a fatal error

orWhereHasMorph('recordable', '*') makes Laravel instantiate every distinct recordable_type found in the table:

// QueriesRelationships::hasMorph()$types = $this->model->newModelQuery()->distinct()->pluck($relation->getMorphType())...
// ...later:$query->where(..., (new$type)->getMorphClass())

Entries are never cleaned up, so as soon as a model is renamed or removed from the app the old rows still hold the old class string:

RecentEntry::create([
'user_id' => $user->id,
'url' => 'https://example.test/gone',
'icon' => '',
'title' => 'Gone',
'recordable_type' => 'App\Models\ThisClassWasDeleted',
'recordable_id' => 1,
]);
RecentEntry::existing()->pluck('url');
Error: Class "App\Models\ThisClassWasDeleted" not found
at vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasRelationships.php:1035
← src/Models/RecentEntry.php:63 (scopeExisting)

That takes down both the menu and global search for the affected user, permanently, until the rows are deleted by hand. Since these entries accumulate forever it's a when-not-if over an app's lifetime. scopeExisting() should resolve the stored types itself and skip any that no longer resolve (Relation::getMorphedModel($type) ?? $type, then class_exists), passing the surviving list to orWhereHasMorph instead of '*'. A test for the stale-type case would be good to have.

Smaller notes

  • The '*' wildcard also runs an unscoped SELECT DISTINCT recordable_type FROM recent_entries — whole table, every user — on each menu render and global search. Indexed by nullableMorphs, so survivable, but it's per-request and resolving the types explicitly (above) lets you cache or narrow it.
  • nullableMorphs respects Schema::defaultMorphKeyType(), so UUID apps are fine if they've set the global default; apps with mixed key types aren't. It's a published migration so users can edit it — worth a line in the README.
  • tests/database/migrations/create_recent_entries_table.php adds nullableMorphs to the create migration, so the actual published add-column migration is never exercised by the suite. Running the real stub would also catch the un-migrated case above.
  • Adding a fourth parameter to Recently::add() breaks any subclass that overrides it, since PHP requires a compatible signature. Unlikely to bite anyone given the facade resolves the concrete class, but flagging it.

If #40 also lands

The two interact: #40 prunes to max_items counting entries whose record is gone, while this PR hides those from display. A user who deletes a lot of records ends up with a near-empty menu while N rows sit in the table. Pruning non-existing entries first would resolve it.

Sign up for freeto 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.

2 participants

@MACscr@awcodes
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Exclude deleted records from recent history - #39

Open
MACscr wants to merge 1 commit into
awcodes:3.xfrom
MACscr:feature/exclude-deleted-records
Open

Exclude deleted records from recent history#39
MACscr wants to merge 1 commit into
awcodes:3.xfrom
MACscr:feature/exclude-deleted-records

Conversation

@MACscr

Copy link
Copy Markdown

Problem

A RecentEntry stores only a URL, so the menu and the global-search "Recently" group keep listing records that have since been deleted. Clicking one lands the user on Filament's "Record not found" page. Any app using the plugin with deletable (including soft-deletable) records hits this.

Change

  • The recorder already holds the viewed $record and discarded it — it's now stored as a nullable polymorphic recordable reference on the entry. add() gains an optional ?Model $record, plus a small backward-compatible migration.
  • A new existing() scope on RecentEntry limits results to entries whose record still resolves — a MorphTo returns null for a soft-deleted or missing record. Both display surfaces apply it: the menu (RecentlyMenu::getRecords()) and global search (RecentEntryResource::getGlobalSearchEloquentQuery()).
  • Entries with no record reference (e.g. non-record pages) are always kept, so pre-existing rows keep displaying.

Notes

  • Backward compatible / SemVer-safe: additive nullable columns, an optional parameter, and an opt-in scope. No public API broken.
  • Existing installs run the published migration (vendor:publish --tag="recently-migrations" + migrate); README updated with an upgrade note and a "Deleted Records" section.
  • Tests added (tests/src/DeletedRecordsTest.php): a soft-deleted record's entry is hidden from both surfaces while live and reference-less entries remain. composer test is green.

Recent entries store only a URL, so the menu and global search kept
listing records that had since been deleted — clicking one lands on
Filament's "Record not found" page.
Store a nullable polymorphic `recordable` reference when recording an
entry (the recorder already has the record), and add an `existing()`
scope that hides entries whose record no longer resolves, including
soft-deleted ones. Both display surfaces apply the scope. Entries with
no record reference are always kept, so existing rows keep displaying.
@awcodes

Copy link
Copy Markdown
Owner

Can you target this to the 2.x branch?

I think it should have Filament v4 support as well.

I'll merge it to 3.x once everything is green.

@awcodes

Copy link
Copy Markdown
Owner

Also, is there a way to allow the end user to allow for soft deletes if they choose to include them?

@awcodesawcodes left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for this — landing on "Record not found" from the menu is a genuine papercut and storing the record the recorder already has is the right fix. Merges cleanly on 3.x and the suite is green. Two things need addressing before this can go in, though, and they change the release it belongs in.

1. Existing installs break on composer update until they migrate

hasMigrations() in laravel-package-tools only registers migrations for publishingrunsMigrations defaults to false and nothing here calls ->runsMigrations():

// vendor/spatie/laravel-package-tools/src/Concerns/Package/HasMigrations.phppublic bool $runsMigrations = false;

So recordable_type / recordable_id don't exist until each user publishes and runs the new migration. In the meantime add() writes both columns and existing() filters on them, which means viewing any record and rendering the menu both throw SQL errors. The README note is accurate, but it's an instruction people will skip, and the failure mode is a broken panel rather than a degraded one.

Two ways out:

  • Guard the write and the scope on Schema::hasColumn('recent_entries', 'recordable_type'), so an un-migrated install behaves exactly as it does today and picks up filtering once it migrates. That keeps this backward compatible in practice, not just at the API level.
  • Or leave it required and ship it in the next major with the migration as a documented upgrade step.

Happy either way, but as written the "SemVer-safe" note in the description holds for the PHP API and not for users.

2. A stale morph type is a fatal error

orWhereHasMorph('recordable', '*') makes Laravel instantiate every distinct recordable_type found in the table:

// QueriesRelationships::hasMorph()$types = $this->model->newModelQuery()->distinct()->pluck($relation->getMorphType())...
// ...later:$query->where(..., (new$type)->getMorphClass())

Entries are never cleaned up, so as soon as a model is renamed or removed from the app the old rows still hold the old class string:

RecentEntry::create([
'user_id' => $user->id,
'url' => 'https://example.test/gone',
'icon' => '',
'title' => 'Gone',
'recordable_type' => 'App\Models\ThisClassWasDeleted',
'recordable_id' => 1,
]);
RecentEntry::existing()->pluck('url');
Error: Class "App\Models\ThisClassWasDeleted" not found
at vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasRelationships.php:1035
← src/Models/RecentEntry.php:63 (scopeExisting)

That takes down both the menu and global search for the affected user, permanently, until the rows are deleted by hand. Since these entries accumulate forever it's a when-not-if over an app's lifetime. scopeExisting() should resolve the stored types itself and skip any that no longer resolve (Relation::getMorphedModel($type) ?? $type, then class_exists), passing the surviving list to orWhereHasMorph instead of '*'. A test for the stale-type case would be good to have.

Smaller notes

  • The '*' wildcard also runs an unscoped SELECT DISTINCT recordable_type FROM recent_entries — whole table, every user — on each menu render and global search. Indexed by nullableMorphs, so survivable, but it's per-request and resolving the types explicitly (above) lets you cache or narrow it.
  • nullableMorphs respects Schema::defaultMorphKeyType(), so UUID apps are fine if they've set the global default; apps with mixed key types aren't. It's a published migration so users can edit it — worth a line in the README.
  • tests/database/migrations/create_recent_entries_table.php adds nullableMorphs to the create migration, so the actual published add-column migration is never exercised by the suite. Running the real stub would also catch the un-migrated case above.
  • Adding a fourth parameter to Recently::add() breaks any subclass that overrides it, since PHP requires a compatible signature. Unlikely to bite anyone given the facade resolves the concrete class, but flagging it.

If #40 also lands

The two interact: #40 prunes to max_items counting entries whose record is gone, while this PR hides those from display. A user who deletes a lot of records ends up with a near-empty menu while N rows sit in the table. Pruning non-existing entries first would resolve it.

Sign up for freeto 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.

2 participants

@MACscr@awcodes