fa/breadcrumb-the-shire
1
0
Fork 1
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix: address code review findings for bookmark feature (H1–L5)

Browse Source 
Fix broken tests (H1), align interface signatures with implementations (H3),
remove dead code (H4), add missing input guard (M1), replace raw $_SESSION
access with SessionStoreInterface (M2), sync update script schema (M4),
use shared getAppBase utility (L2), document fragment stripping (L3),
and apply app- CSS class prefix convention (L5).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Felix Schneider
2026-03-17 22:58:07 +01:00
parent 9688848401
commit d9805c45d3
10 changed files with 195 additions and 91 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
db/updates/2026-03-14-user-bookmarks.sql
View File

@@ -30,6 +30,7 @@ CREATE TABLE IF NOT EXISTS `user_bookmarks` (
PRIMARY KEY (`id`),
UNIQUE KEY `uniq_ub_user_url` (`user_id`, `url`),
KEY `idx_ub_user_sort` (`user_id`, `sort_order`),
KEY `idx_ub_user_group_sort` (`user_id`, `group_id`, `sort_order`),
KEY `idx_ub_group` (`group_id`),
CONSTRAINT `fk_ub_user` FOREIGN KEY (`user_id`) REFERENCES `users` (`id`) ON DELETE CASCADE,
CONSTRAINT `fk_ub_group` FOREIGN KEY (`group_id`) REFERENCES `user_bookmark_groups` (`id`) ON DELETE SET NULL

37
lib/Repository/User/BookmarkGroupRepository.php
View File

@@ -3,50 +3,31 @@
namespace MintyPHP\Repository\User;
use MintyPHP\DB;
use MintyPHP\Repository\Support\RepoQuery;
use MintyPHP\Repository\Support\RepositoryArrayHelper;
class BookmarkGroupRepository implements BookmarkGroupRepositoryInterface
{
private function unwrapList(mixed $rows): array
{
if (!is_array($rows)) {
return [];
}
$list = [];
foreach ($rows as $row) {
$data = $row['user_bookmark_groups'] ?? $row;
if (is_array($data)) {
$list[] = $data;
}
}
return $list;
return RepositoryArrayHelper::unwrapList($rows, 'user_bookmark_groups');
}
public function listByUser(int $userId): array
public function listByUser(int $userId, array $options = []): array
{
if ($userId <= 0) {
return [];
}
[$limit, $offset] = RepoQuery::sanitizeLimitOffset($options, 100, 1, 100);
$rows = DB::select(
'select id, user_id, name, icon, sort_order, created, modified from user_bookmark_groups where user_id = ? order by sort_order, id',
(string) $userId
'select id, user_id, name, icon, sort_order, created, modified from user_bookmark_groups where user_id = ? order by sort_order, id limit ? offset ?',
(string) $userId,
(string) $limit,
(string) $offset
);
return $this->unwrapList($rows);
}
public function nextGroupSortOrder(int $userId): int
{
if ($userId <= 0) {
return 1;
}
$max = DB::selectValue(
'select COALESCE(max(sort_order), 0) from user_bookmark_groups where user_id = ?',
(string) $userId
);
return max(1, ((int) $max) + 1);
}
public function countByUser(int $userId): int
{
if ($userId <= 0) {

9
lib/Repository/User/BookmarkGroupRepositoryInterface.php
View File

@@ -4,10 +4,11 @@ namespace MintyPHP\Repository\User;
interface BookmarkGroupRepositoryInterface
{
/** @return list<array<string, mixed>> */
public function listByUser(int $userId): array;
public function nextGroupSortOrder(int $userId): int;
/**
* @param array{limit?: int, offset?: int} $options
* @return list<array<string, mixed>>
*/
public function listByUser(int $userId, array $options = []): array;
public function countByUser(int $userId): int;

12
lib/Repository/User/BookmarkRepositoryInterface.php
View File

@@ -4,8 +4,11 @@ namespace MintyPHP\Repository\User;
interface BookmarkRepositoryInterface
{
/** @return list<array<string, mixed>> */
public function listByUser(int $userId): array;
/**
* @param array{limit?: int, offset?: int} $options
* @return list<array<string, mixed>>
*/
public function listByUser(int $userId, array $options = []): array;
public function nextBookmarkSortOrder(int $userId, ?int $groupId): int;
@@ -26,5 +29,8 @@ interface BookmarkRepositoryInterface
/** @param list<array{id: int, sort_order: int}> $idOrderPairs */
public function updateSortOrders(int $userId, array $idOrderPairs): bool;
public function clearGroupId(int $groupId, int $userId): bool;
/** @return list<array<string, mixed>> */
public function listByGroup(int $userId, int $groupId): array;
public function unsetGroupId(int $bookmarkId, int $userId, int $sortOrder): bool;
}

12
lib/Support/BookmarkUrlNormalizer.php
View File

@@ -7,10 +7,12 @@ final class BookmarkUrlNormalizer
public static function canonicalizeRelative(string $url): string
{
$url = trim($url);
if ($url === '') {
if ($url === '' || str_contains($url, "\x00")) {
return '';
}
// Strip fragments: they are client-side only and should not
// differentiate bookmarks during deduplication.
$hashPos = strpos($url, '#');
if ($hashPos !== false) {
$url = substr($url, 0, $hashPos);
@@ -20,7 +22,7 @@ final class BookmarkUrlNormalizer
return '';
}
if (preg_match('#^[a-z][a-z0-9+.-]*://#i', $url) || str_starts_with($url, '//')) {
if (preg_match('#^[a-z][a-z0-9+.-]*:#i', $url) || str_starts_with($url, '//')) {
return '';
}
@@ -77,6 +79,7 @@ final class BookmarkUrlNormalizer
$segments = explode('/', $collapsed);
$normalized = [];
$depth = 0;
foreach ($segments as $segment) {
$segment = trim($segment);
@@ -85,11 +88,16 @@ final class BookmarkUrlNormalizer
}
if ($segment === '..') {
if ($depth <= 0) {
return '';
}
array_pop($normalized);
$depth--;
continue;
}
$normalized[] = $segment;
$depth++;
}
return implode('/', $normalized);

13
pages/bookmarks/update-data().php
View File

@@ -22,7 +22,18 @@ if (!Session::checkCsrfToken()) {
$session = app(SessionStoreInterface::class)->all();
$userId = (int) ($session['user']['id'] ?? 0);
if ($userId <= 0) {
http_response_code(401);
Router::json(['ok' => false, 'error' => 'unauthorized']);
return;
}
$bookmarkId = (int) requestInput()->body('id');
if ($bookmarkId <= 0) {
http_response_code(400);
Router::json(['ok' => false, 'error' => 'invalid_bookmark_id']);
return;
}
$data = [];
if (requestInput()->body('name') !== null) {
@@ -37,6 +48,8 @@ $result = $service->updateBookmark($userId, $bookmarkId, $data);
if ($result['ok']) {
app(SessionStoreInterface::class)->set('user_bookmarks', $service->listGroupedForUser($userId));
} else {
http_response_code(400);
}
Router::json($result);

4
templates/partials/app-bookmark-dialog.phtml
View File

@@ -1,13 +1,15 @@
<?php
use MintyPHP\Http\SessionStoreInterface;
use MintyPHP\Service\Bookmark\BookmarkService;
$bmSessionData = is_array($_SESSION['user_bookmarks'] ?? null) ? $_SESSION['user_bookmarks'] : [];
$bmSessionData = is_array(app(SessionStoreInterface::class)->get('user_bookmarks')) ? app(SessionStoreInterface::class)->get('user_bookmarks') : [];
$bmSessionGroups = is_array($bmSessionData['groups'] ?? null) ? $bmSessionData['groups'] : [];
?>
<dialog
data-app-bookmark-dialog
data-app-component="bookmark-save"
data-max-message="<?php e(t('Maximum bookmarks reached')); ?>"
data-title-create="<?php e(t('Bookmarks')); ?>"
data-title-edit="<?php e(t('Bookmarks')); ?>"

108
tests/Service/Bookmark/BookmarkServiceTest.php
View File

@@ -94,27 +94,17 @@ class BookmarkServiceTest extends TestCase
$this->assertSame('updated', $result['mode']);
}
public function testSaveBookmarkUpsertsLegacyBookmarkByCanonicalUrl(): void
public function testSaveBookmarkCreatesWhenNoExactUrlMatch(): void
{
$this->bookmarkRepo->expects($this->once())->method('findByUserAndUrl')->with(1, 'admin/users?a=1&b=2')->willReturn(false);
$this->bookmarkRepo->expects($this->once())->method('listByUser')->with(1)->willReturn([
['id' => 7, 'url' => 'admin/users?b=2&a=1'],
]);
$this->bookmarkRepo->expects($this->once())->method('update')->with(
7,
1,
[
'name' => 'Users',
'group_id' => null,
'url' => 'admin/users?a=1&b=2',
]
)->willReturn(true);
$this->bookmarkRepo->expects($this->never())->method('create');
$this->bookmarkRepo->method('countByUser')->willReturn(0);
$this->navigationRepo->method('nextRootSortOrder')->willReturn(1);
$this->bookmarkRepo->expects($this->once())->method('create')->willReturn(7);
$result = $this->service->saveBookmark(1, 'Users', 'admin/users?a=1&b=2', null);
$this->assertTrue($result['ok']);
$this->assertSame('updated', $result['mode']);
$this->assertSame('created', $result['mode']);
$this->assertSame(7, $result['bookmark']['id']);
}
@@ -338,22 +328,46 @@ class BookmarkServiceTest extends TestCase
$this->assertSame('bi-heart', $result['group']['icon']);
}
public function testDeleteGroupClearsBookmarks(): void
public function testDeleteGroupUnsetsGroupAndDeletes(): void
{
$this->bookmarkRepo->expects($this->once())->method('clearGroupId')->with(5, 1)->willReturn(true);
$this->groupRepo->expects($this->any())->method('delete')->with(5, 1)->willReturn(true);
$this->bookmarkRepo->expects($this->once())->method('listByGroup')->with(1, 5)->willReturn([
['id' => 10, 'name' => 'A', 'url' => 'admin', 'group_id' => 5, 'sort_order' => 1],
['id' => 11, 'name' => 'B', 'url' => 'admin/users', 'group_id' => 5, 'sort_order' => 2],
]);
$this->navigationRepo->expects($this->once())->method('nextRootSortOrder')->with(1)->willReturn(20);
$this->bookmarkRepo->expects($this->exactly(2))->method('unsetGroupId')
->willReturnCallback(fn (int $bmId, int $userId, int $sort) => match ([$bmId, $userId]) {
[10, 1] => $sort === 20,
[11, 1] => $sort === 21,
default => false,
});
$this->groupRepo->expects($this->once())->method('delete')->with(5, 1)->willReturn(true);
$this->assertTrue($this->service->deleteGroup(1, 5));
}
public function testDeleteGroupReturnsFalseWhenClearFails(): void
public function testDeleteGroupReturnsFalseWhenUnsetFails(): void
{
$this->bookmarkRepo->expects($this->once())->method('clearGroupId')->with(5, 1)->willReturn(false);
$this->bookmarkRepo->expects($this->once())->method('listByGroup')->with(1, 5)->willReturn([
['id' => 10, 'name' => 'A', 'url' => 'admin', 'group_id' => 5, 'sort_order' => 1],
]);
$this->navigationRepo->expects($this->once())->method('nextRootSortOrder')->with(1)->willReturn(20);
$this->bookmarkRepo->expects($this->once())->method('unsetGroupId')->with(10, 1, 20)->willReturn(false);
$this->groupRepo->expects($this->never())->method('delete');
$this->assertFalse($this->service->deleteGroup(1, 5));
}
public function testDeleteGroupWithNoBookmarksDeletesDirectly(): void
{
$this->bookmarkRepo->expects($this->once())->method('listByGroup')->with(1, 5)->willReturn([]);
$this->navigationRepo->expects($this->once())->method('nextRootSortOrder')->with(1)->willReturn(1);
$this->bookmarkRepo->expects($this->never())->method('unsetGroupId');
$this->groupRepo->expects($this->once())->method('delete')->with(5, 1)->willReturn(true);
$this->assertTrue($this->service->deleteGroup(1, 5));
}
public function testReorderGroupsDelegatesToRepository(): void
{
$pairs = [
@@ -413,6 +427,60 @@ class BookmarkServiceTest extends TestCase
$this->assertArrayNotHasKey('icon', $result['ungrouped'][0]);
}
public function testReorderFromItemsRejectsInvalidType(): void
{
$result = $this->service->reorderFromItems(1, 'invalid', []);
$this->assertFalse($result['ok']);
$this->assertSame('invalid_type', $result['error']);
}
public function testReorderFromItemsRejectsEmptyItems(): void
{
$result = $this->service->reorderFromItems(1, 'bookmarks', []);
$this->assertFalse($result['ok']);
$this->assertSame('invalid_items', $result['error']);
}
public function testReorderFromItemsBookmarksHappyPath(): void
{
$this->bookmarkRepo->expects($this->once())->method('updateSortOrders')->with(1, [
['id' => 3, 'sort_order' => 1],
['id' => 7, 'sort_order' => 2],
])->willReturn(true);
$result = $this->service->reorderFromItems(1, 'bookmarks', [
['id' => 3, 'sort_order' => 1],
['id' => 7, 'sort_order' => 2],
]);
$this->assertTrue($result['ok']);
}
public function testReorderRootFromItemsHappyPath(): void
{
$this->navigationRepo->expects($this->once())->method('updateRootSortOrders')->with(1, [
['kind' => 'group', 'id' => 2, 'sort_order' => 1],
['kind' => 'bookmark', 'id' => 5, 'sort_order' => 2],
])->willReturn(true);
$result = $this->service->reorderRootFromItems(1, [
['kind' => 'group', 'id' => 2, 'sort_order' => 1],
['kind' => 'bookmark', 'id' => 5, 'sort_order' => 2],
]);
$this->assertTrue($result['ok']);
}
public function testReorderRootFromItemsRejectsEmptyItems(): void
{
$result = $this->service->reorderRootFromItems(1, []);
$this->assertFalse($result['ok']);
$this->assertSame('invalid_items', $result['error']);
}
public function testAllowedGroupIconsReturnsNonEmptyList(): void
{
$icons = BookmarkService::allowedGroupIcons();

2
web/css/components/app-bookmark-form.css
View File

@@ -108,7 +108,7 @@
color: var(--app-primary);
}
.app-bookmark-group-icon-option.active {
.app-bookmark-group-icon-option.app-bookmark-icon-active {
border-color: var(--app-primary);
background: var(--app-primary);
color: #fff;

88
web/js/components/app-bookmark-save.js
View File

@@ -5,29 +5,43 @@
import { showAsyncFlash } from './app-async-flash.js';
import { warnOnce } from '../core/app-dom.js';
import { getAppBase } from '../pages/app-list-utils.js';
export function init() {
const openBtn = document.querySelector('[data-bookmark-open-dialog]');
const bookmarkDialog = document.querySelector('[data-app-bookmark-dialog]');
if (!(bookmarkDialog instanceof HTMLDialogElement) || !(openBtn instanceof HTMLButtonElement)) return;
export function initBookmarkSave(root = document) {
const host = root && typeof root.querySelector === 'function' ? root : document;
const openBtn = host.querySelector('[data-bookmark-open-dialog]') || document.querySelector('[data-bookmark-open-dialog]');
const bookmarkDialog = host.matches?.('[data-app-bookmark-dialog]')
? host
: (host.querySelector('[data-app-bookmark-dialog]') || document.querySelector('[data-app-bookmark-dialog]'));
if (!(bookmarkDialog instanceof HTMLDialogElement) || !(openBtn instanceof HTMLButtonElement)) {
return { destroy: () => {} };
}
if (bookmarkDialog.dataset.bookmarkSaveBound === '1' && bookmarkDialog._bookmarkSaveApi) {
return bookmarkDialog._bookmarkSaveApi;
}
const bookmarkForm = bookmarkDialog.querySelector('[data-bookmark-save-form]');
if (!(bookmarkForm instanceof HTMLFormElement)) return;
if (!(bookmarkForm instanceof HTMLFormElement)) {
return { destroy: () => {} };
}
const groupDialog = document.querySelector('[data-app-bookmark-group-dialog]');
const groupForm = groupDialog instanceof HTMLDialogElement
? groupDialog.querySelector('[data-bookmark-group-save-form]')
: null;
const hasGroupDialog = groupDialog instanceof HTMLDialogElement && groupForm instanceof HTMLFormElement;
const abortController = new AbortController();
const { signal } = abortController;
const timerIds = [];
const root = document.documentElement;
const csrfKey = String(root.dataset.csrfKey || '').trim();
const csrfToken = String(root.dataset.csrfToken || '').trim();
const documentRoot = document.documentElement;
const csrfKey = String(documentRoot.dataset.csrfKey || '').trim();
const csrfToken = String(documentRoot.dataset.csrfToken || '').trim();
const moduleContext = { module: 'bookmark-save' };
if (!csrfKey || !csrfToken) {
warnOnce('UI_DATA_MISSING', 'Missing CSRF metadata for bookmark dialog', moduleContext);
return;
return { destroy: () => {} };
}
const nameInput = bookmarkForm.querySelector('input[name="name"]');
@@ -83,7 +97,8 @@ export function init() {
const runReloadWithSuccess = (message) => {
showAsyncFlash('success', message, 2500);
window.setTimeout(() => window.location.reload(), 450);
const timerId = window.setTimeout(() => window.location.reload(), 450);
timerIds.push(timerId);
};
const closeBookmarkDialog = () => {
@@ -210,33 +225,33 @@ export function init() {
if (openBookmarkDialog() && nameInput instanceof HTMLInputElement) {
nameInput.select();
}
});
}, { signal });
document.addEventListener('app:bookmark-edit', (event) => {
const detail = event instanceof CustomEvent ? event.detail : null;
openBookmarkEditFromSidebar(detail);
});
}, { signal });
closeBookmarkBtns.forEach((btn) => {
btn.addEventListener('click', (event) => {
event.preventDefault();
closeBookmarkDialog();
});
}, { signal });
});
bookmarkDialog.addEventListener('click', (event) => {
if (event.target === bookmarkDialog) {
closeBookmarkDialog();
}
});
}, { signal });
bookmarkDialog.addEventListener('close', () => {
bookmarkForm.removeAttribute('aria-busy');
syncModalBodyClass();
});
}, { signal });
bookmarkDialog.addEventListener('cancel', () => {
bookmarkForm.removeAttribute('aria-busy');
syncModalBodyClass();
});
}, { signal });
// Save bookmark
bookmarkForm.addEventListener('submit', async (event) => {
@@ -310,10 +325,22 @@ export function init() {
} finally {
setBookmarkSubmitPending(false);
}
});
}, { signal });
const destroy = () => {
abortController.abort();
timerIds.forEach((timerId) => window.clearTimeout(timerId));
timerIds.length = 0;
bookmarkForm.removeAttribute('aria-busy');
delete bookmarkDialog.dataset.bookmarkSaveBound;
delete bookmarkDialog._bookmarkSaveApi;
};
const api = { destroy };
bookmarkDialog.dataset.bookmarkSaveBound = '1';
bookmarkDialog._bookmarkSaveApi = api;
if (!hasGroupDialog) {
return;
return api;
}
const groupNameInput = groupForm.querySelector('input[name="name"]');
@@ -381,10 +408,10 @@ export function init() {
groupIconInput.value = value;
}
if (groupIconPicker instanceof HTMLElement) {
groupIconPicker.querySelectorAll('.active').forEach((item) => item.classList.remove('active'));
groupIconPicker.querySelectorAll('.app-bookmark-icon-active').forEach((item) => item.classList.remove('app-bookmark-icon-active'));
const selected = groupIconPicker.querySelector(`[data-icon="${value}"]`);
if (selected instanceof HTMLElement) {
selected.classList.add('active');
selected.classList.add('app-bookmark-icon-active');
}
}
};
@@ -459,7 +486,7 @@ export function init() {
newGroupBtn.addEventListener('click', (event) => {
event.preventDefault();
openCreateGroupFromBookmarkDialog();
});
}, { signal });
}
if (groupIconPicker instanceof HTMLElement) {
@@ -468,29 +495,29 @@ export function init() {
if (!(target instanceof HTMLElement)) return;
event.preventDefault();
setGroupIcon(target.dataset.icon || 'bi-folder');
});
}, { signal });
}
groupCloseBtns.forEach((btn) => {
btn.addEventListener('click', (event) => {
event.preventDefault();
handleGroupCancel();
});
}, { signal });
});
groupDialog.addEventListener('click', (event) => {
if (event.target === groupDialog) {
handleGroupCancel();
}
});
}, { signal });
groupDialog.addEventListener('cancel', (event) => {
event.preventDefault();
handleGroupCancel();
});
}, { signal });
groupDialog.addEventListener('close', () => {
groupForm.removeAttribute('aria-busy');
syncModalBodyClass();
});
}, { signal });
document.addEventListener('app:bookmark-group-edit', (event) => {
const detail = event instanceof CustomEvent ? event.detail : null;
@@ -513,7 +540,7 @@ export function init() {
groupNameInput.focus();
groupNameInput.select();
}
});
}, { signal });
groupForm.addEventListener('submit', async (event) => {
event.preventDefault();
@@ -574,12 +601,9 @@ export function init() {
} finally {
setGroupSubmitPending(false);
}
});
}
}, { signal });
function getAppBase() {
const base = document.querySelector('base');
return base ? base.href : '/';
return api;
}
function currentRelativeUrl() {