Fix stale thumbnails, task list crashes and misleading form options - #2167
Open
NicoPennec wants to merge 26 commits into
Open
Fix stale thumbnails, task list crashes and misleading form options#2167NicoPennec wants to merge 26 commits into
NicoPennec wants to merge 26 commits into
Conversation
npm 11.9.0 to 11.12.1 lose the _isRoot context and refuse a git dependency declared at the root of package.json when allow-git=root (npm/cli#9237, fixed in 11.13.0). The staging server runs npm 11.12.1, where npm ci fails with EALLOWGIT on fabricjs-psbrush. Widen the policy to allow-git=all until that server is upgraded.
Give the task list and the metadata table a minimum height so they no longer collapse when the viewport gets short: the global .data-list and .table-body rules set flex: 1 with a near zero minimum, which let both blocks shrink to nothing. Align Sequence and Episode on the Asset and Shot layout: single flexcolumn info block instead of the leftover single column wrapper, shared toolbar row placed between the tabs and the info block, same .infos and .metadata-infos rules. Drop the dead .flexcolumn rules from the entity and list pages.
The single and bulk tab buttons never set a color, so they fell back to the browser buttontext default and stayed dark in dark mode. Their border was a hardcoded $light-grey too. Both now go through var(--text) and var(--border). The option labelled "Shot Padding" never controlled zero padding: the digit width comes from the name being incremented, and the value is the increment added to it. Rename it to numbering step in the component, in the generateNextName helper and in every locale. The NFT and video game overlays only carried that key to remap the word "Shot" in the label, which the new wording no longer contains, so they drop it. Build the bulk fields with text-field instead of hand written label and input pairs. The widget ties the label to the input, which the raw markup never did, and renders the start error itself. It reports whatever the browser parsed, decimals included, so the preview now asks for whole numbers before it lists anything. Move the numbering step inside the single tab it applies to, give the bulk preview its own scrollable list and let the columns size themselves instead of a fixed height. The .explanation rule is unused now that the intro text relies on the spacing utilities.
The ternary was parsed as (step || type === 'number') ? 'any' : undefined, so every numeric input rendered step="any" and the prop was inert. Six call sites already passed one and were silently ignored: the fps fields of the shot modal, the production modal and the project template settings, the max retakes field of those same settings, and the daily quota and estimation fields of the production schedule. Keep "any" as the default when no step is given, so the call sites that never passed one do not move.
The preview-file:set-main socket event fires for every entity of the production and reaches every connected client. On any page but the episode list, displayedEpisodes is filled by loadEpisodes, whose episodes carry no tasks key, so SET_PREVIEW threw on episode.tasks and aborted the remaining module handlers. displayedSequences has the same two shapes. Also guard task.entity, absent from tasks loaded outside an entity list.
Array.find returns the element, so the lookup yielded the task id string, its entity was always undefined and the guarded assignment was dead code. Iterate the tasks like the shot, edit, sequence and episode modules do.
Setting a non-finite value on a progress element throws, and the crash took the whole render pass down, not just the bar fill. The mouse position was mapped to a frame through width / (nbFrames / zoomLevel). A bar hidden by v-show or not yet measured has a width of 0 and a preview whose metadata is still loading has no frame, so that division was 0 / 0 and every position came out NaN. The drag listeners live on document, so a drag started on the bar keeps firing across a preview switch, right when both values are null. The clamps in getMouseFrame are comparisons, which NaN crosses silently, and setVideoFrameContext guards on currentFrame !== frame, always true for NaN, so the value reached the DOM untouched. Return a zero frame size instead of dividing by zero, skip the drag when the bar has no geometry, and keep viewStartFrame finite: the wheel zoom computed Infinity - Infinity on a zero width bar and latched it to NaN, which then broke the fill for every caller, playback included. Ignore a non-finite frame in updateProgressBar, setCurrentFrame and setVideoFrameContext too. The same NaN can come from onVideoTimeUpdate and would otherwise reach video.currentTime, whose setter throws alike.
Vuex builds its getters with computed(), and taskTypeMap: state => cache.taskTypeMap has no reactive dependency, so it is evaluated once and pinned to the map object it returned first. LOAD_TASK_TYPES_END rebuilt that object while RESET_ALL cleared it in place, so a logout followed by a login in the same tab, without a page reload, left every consumer holding the map the logout had emptied. The task types of the new session went into an object nobody reads. Nothing looks broken at first, since each todo carries its own task_type_name, so the list still renders. The next selected task takes the action panel down instead, on taskTypeMap.get().department_id. Clear and refill the cached maps rather than replacing them, here and in the modules sharing the pattern: task statuses, departments, backgrounds, sequences, people, asset types, hardware items, software licenses and status automations. The person map is emptied in LOAD_PEOPLE_END rather than in LOAD_PEOPLE_START. Rebuilding the map used to hide that emptying, since the start mutation does not bump personMapVersion and consumers kept reading the map of the previous load. Mutated in place it is visible at once, and personMap.get() would return undefined for the whole request on the pages that reload people while showing them.
taskTypeMap.get() returns undefined for a task type that is not loaded or no longer exists, and both copies of the department check read department_id straight off the result. The action panel one crashed on the my tasks page: loadTodos commits REGISTER_USER_TASKS, which registers every todo in taskMap, so the "task not in the store" fallback never fires there and the lookup always runs. The production schedule one is fed by callers passing taskTypeMap.get(item.task_type_id) directly. Deny when the task type cannot be resolved, which is already what the check does for a task type without a department. The task list and the estimation helper resolve it that way too. The fallback comment claimed the branch was the artist todolist, which it stopped being.
loadSharedPlaylistContext committed LOAD_TASK_TYPES_END and LOAD_TASK_STATUSES_END with the production subset carried by the share link, which replaces both sets. Opening a share link in a logged in tab therefore cut the whole session down to that production. The cached maps used to be rebuilt rather than mutated, so the consumers reading them through a getter never saw it, but that shielding is gone. Add the entries the store is missing instead, one EDIT_TASK_TYPE_END or EDIT_TASK_STATUS_END each, and leave the loaded ones untouched. An anonymous visitor still gets the full context, and a logged in one keeps the is_artist_allowed and is_client_allowed values that only the task status load normalises.
LOAD_TASK_TYPES_ERROR is committed nowhere, the loadTaskTypes catch only logs. Its body blanked state.taskTypes and cleared taskTypeMap, which the map rebuild used to hide from every consumer reading the getter. Mutated in place, it would now really empty the map for the whole app whenever a refresh from the task types admin page fails. Wiring it in is not the answer either, a failed reload should leave the task types that were loaded. Drop the mutation, its now unused mutation type and its test.
pendingTasks and notPendingTasks read task.taskStatus.is_feedback_request, a snapshot taken once by USER_LOAD_TODOS_END from the task status map. The map is only filled at boot by loadContext, so a status created later is unknown to it and the snapshot stays undefined, which crashes both computed on render. The task:assign socket handler reloads the todos without refreshing the statuses, and the server memoizes them for two minutes, so even a page reload can hit it. Resolve the status through taskStatusMap instead. A task whose status is missing now falls in notPendingTasks rather than throwing, and the split follows task_status_id, which a status change through a comment does update whereas it leaves the snapshot behind.
The todo, kanban and timesheet lists render their own task objects, loaded by the user and people modules, and they read the flat entity_preview_file_id that no code path was updating. Add the root SET_PREVIEW handler to both modules so those lists follow a new main preview.
The my-checks page keeps its tasks in component state, but they are the very objects REGISTER_USER_TASKS puts in the task map, so sweeping the map on the entity id reaches them. That sweep also covers the todo and done lists, registered the same way, so the user module handler goes away as a duplicate.
The team schedule panel enriches each open task into a copy kept in component state, so no store mutation can reach it. Registering those tasks in the task map was the other option, but the map is enumerated by the hookup playlist, which would then pick up open tasks from any production.
LOAD_ASSETS_END copied the whole task map into state.tasks, a key that is not declared in the module state, has no getter and is read nowhere. On a large production that is a throwaway array of every loaded task, rebuilt on each asset load and never released.
LOAD_PERSON_TASKS_END and REGISTER_USER_TASKS dereference task.last_comment.person_id on every registered task. The three current callers all hit routes that attach the last comment, but any other task payload, the open tasks one for instance, would throw mid-loop and leave the task map half filled.
The tasks module has a second LOAD_PERSON_TASKS_END handler that registers the person todo tasks in its map, so the map sweep already refreshes them. Only the done tasks stay out of it and still need the people module handler.
The people module stopped refreshing the person todo previews now that the map sweep covers them, so the page silently depends on LOAD_PERSON_TASKS_END registering those tasks. Assert the registration and the refresh that follows it.
Render the confirm, edit and group modals with v-if instead of a toggled :active, and make their init watchers immediate so the form is filled on mount.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Solution