From 8f775e34a9b51b07248fcee0ac9c2dba3625fb95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9CNaeel=E2=80=9D?= Date: Sat, 5 Sep 2026 10:00:14 +0300 Subject: [PATCH] Fix review findings in file picker --- HISTORY/2026-09-05-file-picker.md | 33 +++++++++++++++++++- site/app.py | 2 +- upload/frontend/table/add_file_with_dedup.js | 4 +-- upload/frontend/table/init_upload_table.js | 28 ++++++++++++----- upload/frontend/table/render.js | 10 +++--- upload/frontend/table/set_status.js | 9 ++++-- 6 files changed, 67 insertions(+), 19 deletions(-) diff --git a/HISTORY/2026-09-05-file-picker.md b/HISTORY/2026-09-05-file-picker.md index 26b2dca..ff6a4bb 100644 --- a/HISTORY/2026-09-05-file-picker.md +++ b/HISTORY/2026-09-05-file-picker.md @@ -34,7 +34,7 @@ - В таблице реализованы рендер дерева, сворачивание/разворачивание групп, удаление leaf-файлов и удаление групп вместе со всем поддеревом. - Счётчики количества файлов и общего размера пересчитываются после изменений. - Состояние таблицы хранит исходные browser `File` objects и метаданные для последующего сервисного слоя. -- Версия приложения повышена до `0.1.6`. +- Версия приложения повышена до `0.1.7`. ## Исправления по ходу работы @@ -69,6 +69,37 @@ - Дополнительный браузерный тест подтвердил, что ZIP только с `image.jpg` оставляет таблицу пустой: `0 файлов · 0 B`. - Проверено наличие `accept=".pdf,.doc,.docx,.txt,.md,.zip"` у обоих file input. +## Код-ревью и исправления — 2026-09-05 + +Проведено ревью frontend-модулей file-picker после версии `0.1.6`. + +### Найдено + +- **Критично:** удаление файла или группы не удаляло ключ из `state.fileMeta`. После удаления повторный выбор того же файла с тем же путём и размером ошибочно считался дубликатом. +- **Средний приоритет:** для leaf-файлов `padding-left` задавался одновременно на `` и ``, поэтому отступ листьев удваивался. +- **Средний приоритет:** `set_status.js` обращался к удалённому `state.files` и падал с `TypeError` при вызове. Кроме того, старый поиск по имени был бы неоднозначен для одинаковых имён в разных каталогах. +- **Низкий приоритет:** `addFiles` и `onFilesChange` импортировались двумя отдельными строками из одного модуля. + +### Исправлено + +- В `init_upload_table.js` добавлено рекурсивное удаление ключей из `state.fileMeta` перед удалением leaf или группы. Исправлены и UI-обработчик, и публичный `api.remove`. +- В `render.js` убран второй источник отступа для leaf-файлов. +- В `render.js` для файлов добавлен `data-path` с экранированным полным путём. +- В `set_status.js` переход выполнен на актуальный `state.nodes` через `flattenFiles`; строка статуса ищется по полному `data-path`. +- Объединён дублирующий импорт из `on_files_change.js`. +- `state.fileMeta` заменён на `Set state.fileKeys`, поскольку значения идентификаторов узлов не использовались. +- Логика удаления узла объединена в одну функцию и используется обработчиком таблицы и публичным `api.remove`. +- Из `findNode` удалён неиспользуемый параметр `parent`. + +### Проверено после ревью + +- Все frontend `.js` файлы проходят `node --input-type=module --check`. +- `site/app.py` проходит `python3 -m py_compile`. +- `git diff --check` проходит. +- Браузерная проверка удаления leaf-файла показала `0 файлов · 0 B`; после удаления ключ дедупликации очищается. +- Повторный выбор того же файла после удаления снова разрешён. +- Изменения не затрагивают фильтрацию расширений, ZIP-рекурсию и публичный API picker. + ## Git - `c0b8788 Implement reusable upload platform` — первоначальный reusable upload platform. diff --git a/site/app.py b/site/app.py index f475b66..96b6268 100644 --- a/site/app.py +++ b/site/app.py @@ -9,7 +9,7 @@ with (ROOT / "config.json").open(encoding="utf-8") as config_file: CONFIG = json.load(config_file) -VERSION = "0.1.6" +VERSION = "0.1.7" app = Flask(__name__, template_folder="templates", static_folder="static") diff --git a/upload/frontend/table/add_file_with_dedup.js b/upload/frontend/table/add_file_with_dedup.js index 35992f3..6fdbfb0 100644 --- a/upload/frontend/table/add_file_with_dedup.js +++ b/upload/frontend/table/add_file_with_dedup.js @@ -2,8 +2,8 @@ export function addFileWithDedup(state, fileNode) { const accept = (node) => { if (node.kind === 'file') { const key = `${node.path}\u0000${node.file.size}`; - if (state.fileMeta.has(key)) return null; - state.fileMeta.set(key, node.id); + if (state.fileKeys.has(key)) return null; + state.fileKeys.add(key); return node; } node.children = node.children.map(accept).filter(Boolean); diff --git a/upload/frontend/table/init_upload_table.js b/upload/frontend/table/init_upload_table.js index f23c1e7..b8b051b 100644 --- a/upload/frontend/table/init_upload_table.js +++ b/upload/frontend/table/init_upload_table.js @@ -1,8 +1,22 @@ -import { addFiles } from './on_files_change.js'; -import { onFilesChange } from './on_files_change.js'; +import { addFiles, onFilesChange } from './on_files_change.js'; import { onFolderChange } from './on_folder_change.js'; import { findNode, flattenFiles, render } from './render.js'; +function removeFileMeta(state, node) { + if (node.kind === 'file') { + state.fileKeys.delete(`${node.path}\u0000${node.file.size}`); + return; + } + node.children.forEach((child) => removeFileMeta(state, child)); +} + +function removeNode(state, id) { + const found = findNode(state.nodes, id); + if (!found) return; + removeFileMeta(state, found.node); + found.nodes.splice(found.nodes.indexOf(found.node), 1); +} + export function initUploadTable(cfg) { const elements = { fileInputEl: cfg.fileInputEl, @@ -10,7 +24,7 @@ export function initUploadTable(cfg) { tableBodyEl: cfg.tableBodyEl, countEl: cfg.countEl, }; - const state = { nodes: [], fileMeta: new Map(), busy: false }; + const state = { nodes: [], fileKeys: new Set(), busy: false }; elements.fileInputEl.addEventListener('change', onFilesChange(state, cfg, elements)); elements.folderInputEl.addEventListener('change', onFolderChange(state, cfg, elements)); elements.tableBodyEl.addEventListener('click', (event) => { @@ -23,8 +37,7 @@ export function initUploadTable(cfg) { } const remove = event.target.closest('[data-remove]'); if (remove) { - const found = findNode(state.nodes, remove.dataset.remove); - if (found) found.nodes.splice(found.nodes.indexOf(found.node), 1); + removeNode(state, remove.dataset.remove); render(state, elements); } }); @@ -35,14 +48,13 @@ export function initUploadTable(cfg) { addFiles: (files) => addFiles(state, cfg, files, elements), getFiles: () => flattenFiles(state.nodes), remove: (id) => { - const found = findNode(state.nodes, id); - if (found) found.nodes.splice(found.nodes.indexOf(found.node), 1); + removeNode(state, id); render(state, elements); }, render: () => render(state, elements), clear: () => { state.nodes = []; - state.fileMeta.clear(); + state.fileKeys.clear(); render(state, elements); }, setBusy: (busy) => { diff --git a/upload/frontend/table/render.js b/upload/frontend/table/render.js index b8ca129..ba7628c 100644 --- a/upload/frontend/table/render.js +++ b/upload/frontend/table/render.js @@ -9,9 +9,9 @@ function renderNode(node, depth) { ? `` : `${name}`; - const indent = isGroup ? '' : ` style="padding-left:${padding}ch"`; const remove = ``; - const row = `${action}` + const pathAttribute = node.kind === 'file' ? ` data-path="${esc(node.path)}"` : ''; + const row = `${action}` + `${node.kind === 'file' ? fs(node.file.size) : ''}` + `${node.error ? esc(node.error) : node.kind === 'file' ? 'готов' : ''}${remove}`; if (!isGroup || !node.expanded) return row; @@ -31,10 +31,10 @@ export function render(state, elements) { elements.countEl.textContent = `${files.length} файлов · ${fs(files.reduce((sum, node) => sum + node.file.size, 0))}`; } -export function findNode(nodes, id, parent = null) { +export function findNode(nodes, id) { for (const node of nodes) { - if (node.id === id) return { node, parent, nodes }; - const found = findNode(node.children || [], id, node); + if (node.id === id) return { node, nodes }; + const found = findNode(node.children || [], id); if (found) return found; } return null; diff --git a/upload/frontend/table/set_status.js b/upload/frontend/table/set_status.js index 5f91ab3..f9cb2b8 100644 --- a/upload/frontend/table/set_status.js +++ b/upload/frontend/table/set_status.js @@ -1,5 +1,10 @@ +import { flattenFiles } from './render.js'; + export function setStatus(path, html, state, elements) { - const index = state.files.findIndex((file) => file.name === path); - const cell = elements.tableBodyEl.querySelector(`#st-${index}`); + const file = flattenFiles(state.nodes).find((item) => item.path === path); + if (!file) return; + const cell = Array.from(elements.tableBodyEl.querySelectorAll('.tree-row.tree-file')) + .find((row) => row.dataset.path === file.path) + ?.querySelector('td:nth-child(3)'); if (cell) cell.innerHTML = html; } \ No newline at end of file