Fix review findings in file picker
This commit is contained in:
“Naeel”
@@ -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` задавался одновременно на `<td>` и `<span>`, поэтому отступ листьев удваивался.
|
||||
- **Средний приоритет:** `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.
|
||||
|
||||
+1
-1
@@ -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")
|
||||
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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) => {
|
||||
|
||||
@@ -9,9 +9,9 @@ function renderNode(node, depth) {
|
||||
? `<button class="tree-name tree-group tree-${node.kind}" style="padding-left:${padding}ch" data-toggle="${node.id}" `
|
||||
+ `aria-expanded="${node.expanded}"><span class="tree-chevron">${node.expanded ? '▼' : '▶'}</span>${name}</button>`
|
||||
: `<span class="tree-name" style="padding-left:${padding}ch">${name}</span>`;
|
||||
const indent = isGroup ? '' : ` style="padding-left:${padding}ch"`;
|
||||
const remove = `<button class="remove-btn" type="button" data-remove="${node.id}" aria-label="Удалить ${name}">×</button>`;
|
||||
const row = `<tr class="tree-row tree-${node.kind}"><td${indent}>${action}</td>`
|
||||
const pathAttribute = node.kind === 'file' ? ` data-path="${esc(node.path)}"` : '';
|
||||
const row = `<tr class="tree-row tree-${node.kind}"${pathAttribute}><td>${action}</td>`
|
||||
+ `<td>${node.kind === 'file' ? fs(node.file.size) : ''}</td>`
|
||||
+ `<td>${node.error ? esc(node.error) : node.kind === 'file' ? 'готов' : ''}</td><td>${remove}</td></tr>`;
|
||||
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;
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
Reference in New Issue
Block a user