feat: BP Navigation Stack [AB-897] #1249
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR implements a blueprint page navigation stack with forward/backward navigation, introduces a new BlueprintsNavigationStack UI component with keyboard shortcuts, adds double-click handling on blueprint nodes to open editors or navigate, and coordinates code editor expansion state across components via a shared expandedEditorForComponent reactive property. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant BlueprintsNav as BlueprintsNavigationStack
participant Core
participant BlueprintsRoot
participant BuilderManager
participant DOM as DOM/Component Tree
User->>BlueprintsNav: Click forward/backward or<br/>keyboard shortcut (Ctrl+Shift)
BlueprintsNav->>Core: navigateInPageStack("forward"|"backward")
activate Core
Core->>Core: Update pageStack index
Core->>Core: Set activePageId to new page
deactivate Core
Core-->>BlueprintsNav: Updated activePageId
BlueprintsNav->>BuilderManager: clearSelection()
BuilderManager-->>DOM: Clear selected components
BlueprintsNav->>DOM: Update displayed blueprint
sequenceDiagram
actor User
participant BlueprintsNode
participant BuilderManager
participant BuilderSettingsProperties as BuilderSettingsProperties<br/>(Field Editor)
participant Editor as BuilderEmbeddedCodeEditor
User->>BlueprintsNode: Double-click on code component
BlueprintsNode->>BlueprintsNode: Check if isCodeComponent
BlueprintsNode->>BuilderManager: Set expandedEditorForComponent<br/>to component.id
BuilderManager-->>BuilderSettingsProperties: Notify expandedEditorForComponent change
activate BuilderSettingsProperties
BuilderSettingsProperties->>BuilderSettingsProperties: Compute isFieldExpanded=true
BuilderSettingsProperties->>BuilderSettingsProperties: Trigger expansion via :should-expand
deactivate BuilderSettingsProperties
BuilderSettingsProperties->>Editor: Mount/show editor
Editor->>Editor: Auto-focus & move cursor<br/>to last line (half-screen)
Editor-->>User: Display focused editor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
HackerOne Code Security Review🟢 Scan Complete: 7 Issue(s) Here's how the code changes were interpreted and info about the tools used for scanning. 📖 Summary of ChangesThe changes primarily focus on enhancing the builder and blueprints navigation experience. Key modifications include adding an expanded editor tracking mechanism, implementing a navigation stack for blueprints, and introducing new interaction methods like double-click navigation and keyboard shortcuts. The updates improve component management, editor interactions, and provide more flexible navigation between different views and components.
ℹ️ Issues DetectedNOTE: These may not require action! Below are unvalidated results from the Analysis Tools that ran during the latest scan for transparency. We investigate each of these for accuracy and relevance before surfacing them as a potential problem. How will I know if something is a problem?
🧰 Analysis tools
⏱️ Latest scan covered changes up to commit 8f20fe9 (latest) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/ui/src/wds/WdsFieldWrapper.vue (1)
112-120: Consider handling the shrink case.The watcher only triggers expansion when
shouldExpandbecomes true. IfshouldExpandis externally set to false while the field is expanded, the UI won't collapse. Consider whether bidirectional synchronization is needed.🔎 Optional enhancement for bidirectional sync
// Watch for external control of expansion watch( () => props.shouldExpand, (newVal) => { if (newVal && !isExpanded.value) { handleExpansion(); + } else if (!newVal && isExpanded.value) { + isExpanded.value = false; + emits("shrink"); } }, );src/ui/src/builder/sidebar/BuilderSidebarComponentTree.vue (1)
179-191: Unusual fallback value in key generation.The fallback calculation uses
allBlueprints.value.length + 10when no existingBLUEPRINT_keys are found. The+ 10offset seems arbitrary. Consider whether this should simply be1to start the sequence, or if there's a specific reason for the offset.🔎 Proposed simplification
function getNewBlueprintKey() { const existingKeys = allBlueprints.value .map((bp) => bp.content?.key) .filter((key) => key?.startsWith("BLUEPRINT_")) .map((key) => Number.parseInt(key.replace("BLUEPRINT_", ""), 10)) .filter((num) => !Number.isNaN(num)); const maxIndex = existingKeys.length > 0 ? Math.max(...existingKeys) - : allBlueprints.value.length + 10; + : 0; return `BLUEPRINT_${maxIndex + 1}`; }src/ui/src/components/blueprints/abstract/BlueprintsNode.vue (1)
182-184: Consider expandingisCodeComponentto matchhandleDoubleClickconditions.The
isCodeComponentcomputed only checks forblueprints_code, buthandleDoubleClickalso handlesblueprints_setstate,blueprints_returnvalue, andblueprints_logmessagefor editor expansion. This means the "open editor" action button (controlled byshowOpenEditorOption) won't appear for those other component types, which may be inconsistent with the double-click behavior.🔎 Proposed fix to align the computed with handleDoubleClick logic
const isCodeComponent = computed(() => { - return component.value?.type === "blueprints_code"; + return ( + component.value?.type === "blueprints_code" || + component.value?.type === "blueprints_setstate" || + component.value?.type === "blueprints_returnvalue" || + component.value?.type === "blueprints_logmessage" + ); });src/ui/src/core/index.ts (1)
948-1003: Inconsistent handling ofactivePageIdinnavigateInPageStackwarrants refactoring.The function updates
activePageIddirectly whenpageId === null(lines 993-996 and 1000) but delegates to the caller whenpageId !== null. This asymmetry is mitigated currently becausegetComponentAndNavigateis only called fromsetActivePageId, which handles the update sequentially (line 915). However, the inconsistent API design creates maintenance risk—consider refactoring to always or never updateactivePageIdwithin the function to make the behavior explicit and prevent future misuse.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
src/ui/src/builder/BuilderApp.vuesrc/ui/src/builder/BuilderEmbeddedCodeEditor.vuesrc/ui/src/builder/builderManager.tssrc/ui/src/builder/settings/BuilderSettingsProperties.vuesrc/ui/src/builder/sidebar/BuilderSidebarComponentTree.vuesrc/ui/src/components/blueprints/BlueprintsNavigationStack.vuesrc/ui/src/components/blueprints/BlueprintsRoot.vuesrc/ui/src/components/blueprints/abstract/BlueprintsNode.vuesrc/ui/src/components/blueprints/abstract/BlueprintsNodeActions.vuesrc/ui/src/composables/useWriterTracking.tssrc/ui/src/core/index.tssrc/ui/src/wds/WdsFieldWrapper.vuesrc/ui/src/writerTypes.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-05T12:07:52.320Z
Learnt from: VladKrasny
Repo: writer/writer-framework PR: 1059
File: src/ui/src/builder/settings/BuilderFieldsAlign.vue:27-27
Timestamp: 2025-08-05T12:07:52.320Z
Learning: In Vue 3 templates, refs are automatically unwrapped, so when binding a ref to a template property, you should use the ref directly (e.g., `:value="fieldViewModel"`) instead of explicitly accessing the .value property (e.g., `:value="fieldViewModel.value"`). The .value access is only needed in script contexts.
Applied to files:
src/ui/src/wds/WdsFieldWrapper.vuesrc/ui/src/components/blueprints/abstract/BlueprintsNode.vue
📚 Learning: 2025-08-05T12:07:52.320Z
Learnt from: VladKrasny
Repo: writer/writer-framework PR: 1059
File: src/ui/src/builder/settings/BuilderFieldsAlign.vue:27-27
Timestamp: 2025-08-05T12:07:52.320Z
Learning: In Vue 3 templates, refs are automatically unwrapped, so when binding a ref to a template property, you can use the ref directly (e.g., `:value="fieldViewModel"`) instead of explicitly accessing the .value property (e.g., `:value="fieldViewModel.value"`).
Applied to files:
src/ui/src/wds/WdsFieldWrapper.vue
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: tests (chromium)
- GitHub Check: tests (webkit)
- GitHub Check: tests (firefox)
- GitHub Check: build (3.10)
- GitHub Check: build (3.13)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.9)
🔇 Additional comments (30)
src/ui/src/writerTypes.ts (1)
282-285: LGTM! Clean type definition for navigation stack.The new
NavigationStackItemtype is well-defined with appropriate typing. UsingComponent["id"]maintains consistency with existing component references throughout the codebase.src/ui/src/wds/WdsFieldWrapper.vue (2)
4-6: LGTM! Template formatting corrected.The WdsModal usage has been properly formatted to ensure the slot content is correctly placed inside the modal body.
84-97: LGTM! External expansion control added.The new
shouldExpandprop enables external control of field expansion, which integrates well with the sharedexpandedEditorForComponentstate introduced elsewhere in this PR.src/ui/src/builder/sidebar/BuilderSidebarComponentTree.vue (1)
157-177: LGTM! Blueprint key generation integrated.The reordering of activation after
nextTickis good practice, ensuring DOM/state updates complete before setting the active page. The key generation and assignment logic is clean.src/ui/src/builder/BuilderApp.vue (1)
270-275: LGTM! Escape key now clears expanded editor state.Adding the expanded editor state clearing on Escape keydown provides consistent UX, ensuring both selection and expanded editors are cleared together.
src/ui/src/components/blueprints/abstract/BlueprintsNodeActions.vue (3)
2-2: LGTM! Imports and prop for new editor button.The
nextTickimport andshowOpenEditorOptionprop are correctly added to support the new code editor button functionality.Also applies to: 25-25
43-48: LGTM! Code editor expansion handler.The
openEditorForComponentfunction correctly coordinates selection, DOM updates vianextTick, editor expansion, and analytics tracking. The flow aligns well with the sharedexpandedEditorForComponentstate management introduced in this PR.
107-117: LGTM! New code editor button.The button is properly guarded by
showOpenEditorOption, includes appropriate data attributes for tooltips and unselectability, and uses the correct icon.src/ui/src/composables/useWriterTracking.ts (1)
45-48: LGTM! Tracking events expanded.The new event literals correctly extend the tracking system to support the navigation stack and editor expansion interactions introduced in this PR.
src/ui/src/components/blueprints/BlueprintsRoot.vue (3)
3-3: LGTM! Navigation stack component integrated.The
BlueprintsNavigationStackcomponent replaces the automatic page synchronization logic, providing explicit navigation controls with forward/backward navigation as described in the PR objectives.Also applies to: 30-32
64-90: LGTM! Title container styling added.The new CSS blocks provide appropriate styling for the navigation stack title area with proper positioning, spacing, and visual separation.
30-30: Initial page state is properly handled through computed properties.The
displayedBlueprintIdcomputed property in bothBlueprintsRootandBlueprintsNavigationStackreactively determines which blueprint to display based on the active page ID. Vue's reactivity system automatically evaluates this property on mount and re-evaluates whenever the underlying state changes, eliminating the need for explicitonMountedsynchronization logic. The navigation stack'sonMountedhook handles only keyboard shortcuts.Likely an incorrect or invalid review comment.
src/ui/src/builder/BuilderEmbeddedCodeEditor.vue (1)
119-130: Monaco Editor API methods are available and correctly implemented.The methods
getLineCount()andgetLineLastNonWhitespaceColumn()are standard ITextModel API methods available in all supported Monaco Editor versions. The auto-focus and cursor positioning enhancement correctly uses these methods to place the cursor at the last non-whitespace position of the final line when the modal expands.src/ui/src/builder/builderManager.ts (2)
417-428: LGTM - Inline refactoring preserves original logic.The collapsed expressions for
runIdextraction andisActivedetermination are cleaner while maintaining the same behavior. The optional chaining and Boolean coercion are appropriate.
438-438: LGTM - New shared state for editor expansion.The
expandedEditorForComponentref provides a single source of truth for tracking which component's editor is expanded, enabling coordination across UI components.src/ui/src/builder/settings/BuilderSettingsProperties.vue (3)
292-296: LGTM - Computed property for expansion state coordination.The
isFieldExpandedcomputed correctly derives its value from the sharedexpandedEditorForComponentstate, enabling synchronized expansion across components.
93-93: LGTM - External expansion control binding.The
:should-expandbinding correctly combinesisExpansiblecheck with the newisFieldExpandedstate to drive field wrapper expansion from external triggers.
401-409: LGTM - Proper state synchronization in handlers.The
handleExpandandhandleShrinkfunctions correctly update the sharedexpandedEditorForComponentstate alongside the localexpandedFieldsSet, maintaining consistency between the global and local expansion tracking.src/ui/src/core/index.ts (4)
90-90: LGTM - Page stack state initialization.The
pageStackref correctly initializes as an empty array to hold navigation history.
913-916: LGTM - Delegation to navigation logic.The
setActivePageIdnow correctly delegates togetComponentAndNavigatebefore updating the active page, ensuring navigation stack is properly maintained.
926-946: LGTM - Initial stack seeding logic.The
getComponentAndNavigatecorrectly seeds the navigation stack with the first blueprint when the stack is empty and no active page exists, preventing loss of the starting point in navigation history.
1080-1085: LGTM - Navigation API exposure.The new navigation helpers are correctly exposed in the core's public API, enabling the navigation stack UI to query and control navigation state.
src/ui/src/components/blueprints/abstract/BlueprintsNode.vue (2)
14-14: LGTM - Double-click event binding.The
@dblclickhandler is correctly bound to enable the new double-click interactions for editor expansion and blueprint navigation.
424-460: LGTM - Double-click handler with appropriate UX flows.The handler correctly differentiates between code-related components (expanding the editor when selected) and
blueprints_runblueprint(navigating to the target blueprint with selection and toast feedback). Analytics tracking is appropriately integrated for both paths.One minor observation: the tracking event uses
isCodeComponent.valueto distinguish between "code editor" and "value" events, but given the currentisCodeComponentdefinition only coversblueprints_code, the ternary will always resolve to"dbl_click_for_code_editor_opened"when inside the firstifblock. This is correct but worth noting ifisCodeComponentis updated per the earlier suggestion.src/ui/src/components/blueprints/BlueprintsNavigationStack.vue (6)
1-45: LGTM - Clean navigation UI template.The template correctly renders navigation controls with proper disabled states, tooltips, and conditional display. The
data-writer-unselectableattribute and tooltip placements are appropriately configured.
57-76: LGTM - Blueprint resolution logic.The
displayedBlueprintIdcorrectly falls back to the first blueprint when the active page is not under the blueprints root, anddisplayedBlueprintKeyproperly extracts the key from the component content.
94-104: LGTM - Platform detection with graceful fallback.The
isMacdetection correctly tries the modern User-Agent Client Hints API first and falls back to parsing theuserAgentstring for broader compatibility.
106-122: Note: Tooltips depend on buggy helper functions.The tooltips use
getPreviousPageInPageStackandgetNextPageInPageStackwhich currently have bugs incore/index.ts(returning the current page instead of adjacent pages). Once those are fixed, these tooltips will work correctly.
142-162: LGTM - Keyboard shortcut handling.The keyboard shortcut logic correctly handles both
Ctrl+-for back andCtrl+Shift+-(which produces_on most keyboards) for forward navigation. The comment about Shift+- producing "_" is helpful.
164-170: LGTM - Proper event listener lifecycle management.The keyboard event listener is correctly added on mount and removed on unmount, preventing memory leaks.
| const selectedItemKey = computed(() => { | ||
| if (wfbm.selection.value.length > 1) { | ||
| return null; | ||
| } | ||
| const { component, definition: def } = useComponentInformation( | ||
| wf, | ||
| wfbm.firstSelectedId, | ||
| ); | ||
| if (!component.value || component.value.type === "blueprints_blueprint") { | ||
| return null; | ||
| } | ||
| return component.value?.content?.alias || def.value?.name || "Unknown"; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid calling composables inside computed properties.
Calling useComponentInformation inside a computed property is an anti-pattern in Vue 3. Composables should be called at the top level of setup() to ensure proper lifecycle management and reactivity. The current approach will create new reactive objects on every re-evaluation.
🔎 Proposed fix
const wf = inject(injectionKeys.core);
const wfbm = inject(injectionKeys.builderManager);
+const { component: selectedComponent, definition: selectedDef } =
+ useComponentInformation(wf, wfbm.firstSelectedId);
+
const displayedBlueprintId = computed(() => {
// ... existing code
});
const selectedItemKey = computed(() => {
if (wfbm.selection.value.length > 1) {
return null;
}
- const { component, definition: def } = useComponentInformation(
- wf,
- wfbm.firstSelectedId,
- );
- if (!component.value || component.value.type === "blueprints_blueprint") {
+ if (!selectedComponent.value || selectedComponent.value.type === "blueprints_blueprint") {
return null;
}
- return component.value?.content?.alias || def.value?.name || "Unknown";
+ return selectedComponent.value?.content?.alias || selectedDef.value?.name || "Unknown";
});Based on learnings, in Vue 3 templates, refs are automatically unwrapped, so the .value access is only needed in script contexts as shown here.
🤖 Prompt for AI Agents
In src/ui/src/components/blueprints/BlueprintsNavigationStack.vue around lines
78-92, the computed property calls the composable useComponentInformation inside
it which creates new reactive state on each re-evaluation; move the call to
useComponentInformation to the top level of setup() (before any computed) and
capture its returned refs (component, definition) there, then update
selectedItemKey to read those refs (and wfbm refs) without re-invoking the
composable; also simplify .value usage in template/script contexts as needed so
the computed only reads existing refs and returns the alias/name/Unknown.
| function getPreviousPageInPageStack() { | ||
| return pageStack.value.find((item) => item.id === activePageId.value); | ||
| } | ||
|
|
||
| function getNextPageInPageStack() { | ||
| return pageStack.value.find((item) => item.id === activePageId.value); | ||
| } |
There was a problem hiding this comment.
Bug: getPreviousPageInPageStack and getNextPageInPageStack return the current page instead of adjacent pages.
Both functions have identical implementations that return the current active page rather than the previous/next pages in the stack. This will cause the tooltips in BlueprintsNavigationStack to display incorrect information.
🔎 Proposed fix
function getPreviousPageInPageStack() {
- return pageStack.value.find((item) => item.id === activePageId.value);
+ const currentIdx = pageStack.value.findIndex(
+ (item) => item.id === activePageId.value,
+ );
+ if (currentIdx > 0) {
+ return pageStack.value[currentIdx - 1];
+ }
+ return undefined;
}
function getNextPageInPageStack() {
- return pageStack.value.find((item) => item.id === activePageId.value);
+ const currentIdx = pageStack.value.findIndex(
+ (item) => item.id === activePageId.value,
+ );
+ if (currentIdx + 1 < pageStack.value.length) {
+ return pageStack.value[currentIdx + 1];
+ }
+ return undefined;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function getPreviousPageInPageStack() { | |
| return pageStack.value.find((item) => item.id === activePageId.value); | |
| } | |
| function getNextPageInPageStack() { | |
| return pageStack.value.find((item) => item.id === activePageId.value); | |
| } | |
| function getPreviousPageInPageStack() { | |
| const currentIdx = pageStack.value.findIndex( | |
| (item) => item.id === activePageId.value, | |
| ); | |
| if (currentIdx > 0) { | |
| return pageStack.value[currentIdx - 1]; | |
| } | |
| return undefined; | |
| } | |
| function getNextPageInPageStack() { | |
| const currentIdx = pageStack.value.findIndex( | |
| (item) => item.id === activePageId.value, | |
| ); | |
| if (currentIdx + 1 < pageStack.value.length) { | |
| return pageStack.value[currentIdx + 1]; | |
| } | |
| return undefined; | |
| } |
🤖 Prompt for AI Agents
In src/ui/src/core/index.ts around lines 1023 to 1029, both
getPreviousPageInPageStack and getNextPageInPageStack currently return the
active page because they use identical find logic; update them to locate the
index of the activePageId in pageStack.value, then return pageStack.value[index
- 1] for previous and pageStack.value[index + 1] for next, with proper boundary
checks (return undefined/null when no adjacent page exists) and avoid using find
by id for the adjacent lookup.
|
✅ Graham C reviewed all the included code changes and associated automation findings and determined that there were no immediately actionable security flaws. Note that they will continue to be notified of any new commits or comments and follow up as needed throughout the duration of this pull request's lifecycle.
Reviewed with ❤️ by PullRequest |
| return ( | ||
| pageStack.value.findIndex( | ||
| (item) => item.id === activePageId.value, | ||
| ) + | ||
| 1 < | ||
| pageStack.value.length | ||
| ); |
There was a problem hiding this comment.
| return ( | |
| pageStack.value.findIndex( | |
| (item) => item.id === activePageId.value, | |
| ) + | |
| 1 < | |
| pageStack.value.length | |
| ); | |
| return getNextPageInPageStack() !== undefined; |
| | "dbl_click_for_code_editor_opened" | ||
| | "dbl_click_for_value_opened" | ||
| | "button_click_for_code_editor_opened" | ||
| | "dbl_click_for_blueprint_navigated"; |
There was a problem hiding this comment.
It should be prefixed by blueprint to follow the convention
| | "dbl_click_for_code_editor_opened" | |
| | "dbl_click_for_value_opened" | |
| | "button_click_for_code_editor_opened" | |
| | "dbl_click_for_blueprint_navigated"; | |
| | "blueprints_dbl_click_for_code_editor_opened" | |
| | "blueprints_dbl_click_for_value_opened" | |
| | "blueprints_button_click_for_code_editor_opened" | |
| | "blueprints_dbl_click_for_blueprint_navigated"; |
There was a problem hiding this comment.
This file become too big. I think it could be beneficial to move this logic into an internal new hook
export function useNavigationStack(activePageId) {
// ....
return {
navigateInPageStack,
canGoBackInPageStack,
canGoForwardInPageStack,
getPreviousPageInPageStack,
getNextPageInPageStack,
}
}and then you instantiate here
const {
navigateInPageStack,
canGoBackInPageStack,
canGoForwardInPageStack,
getPreviousPageInPageStack,
getNextPageInPageStack,
} = useNavigationStack()It'll also help you to add tests
| export type NavigationStackItem = { | ||
| id: Component["id"]; | ||
| key: string; | ||
| }; |
There was a problem hiding this comment.
Since it's an internal type, I think it can stay in src/ui/src/core/index.ts (or in the new hook as proposed)
| } else { | ||
| // if pageId is null, navigate to the next page in the stack | ||
| if (currentIdx + 1 < pageStack.value.length) { | ||
| activePageId.value = pageStack.value[currentIdx + 1].id; | ||
| } | ||
| } |
There was a problem hiding this comment.
| } else { | |
| // if pageId is null, navigate to the next page in the stack | |
| if (currentIdx + 1 < pageStack.value.length) { | |
| activePageId.value = pageStack.value[currentIdx + 1].id; | |
| } | |
| } | |
| } else if (currentIdx + 1 < pageStack.value.length) { | |
| // if pageId is null, navigate to the next page in the stack | |
| activePageId.value = pageStack.value[currentIdx + 1].id; | |
| } |
| } | ||
|
|
||
| function deleteComponent(componentId: Component["id"]) { | ||
| delete components.value[componentId]; |
There was a problem hiding this comment.
You might want to update pageStack here if the user deletes the page.
|
Graham C has submitted feedback.
Reviewed with ❤️ by PullRequest |
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
| } | ||
| } | ||
| onMounted(() => { |
There was a problem hiding this comment.
The keyboard shortcut handler is registered globally on the window without checking user context. It intercepts Ctrl+- and Ctrl+Shift+- combinations even when users are typing in input fields, textareas, or contenteditable elements. This prevents users from typing these key combinations as part of legitimate text entry, such as when naming blueprints or entering descriptions.
Global keyboard handlers that don't respect input context create accessibility issues and poor user experience. This violates WCAG guidelines on character key shortcuts, which require that shortcuts either don't interfere with text input or can be disabled.
Remediation:
Add context checking at the start of handleKeyboardShortcut (line 140):
function handleKeyboardShortcut(event: KeyboardEvent) {
const target = event.target as HTMLElement;
if (target.matches('input, textarea, [contenteditable="true"]')) {
return;
}
// existing code...
}This allows shortcuts in the main UI while preserving normal keyboard behavior in text entry contexts.
References:
- https://www.w3.org/WAI/WCAG21/Understanding/character-key-shortcuts.html
- https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent
- https://web.dev/keyboard-access/
🔺 Bug (Error)
| let pendingComponentUpdate = false; | ||
|
|
||
| const activePageId = ref<Component["id"] | undefined>(); | ||
| const pageStack = ref<NavigationStackItem[]>([]); |
There was a problem hiding this comment.
The pageStack array has no maximum size limit, allowing unbounded growth during long user sessions. Each navigation to a blueprint adds an entry, and there's no cleanup mechanism. A user navigating through many blueprints could accumulate hundreds of entries, causing memory exhaustion and browser performance degradation.
This violates OWASP guidelines on resource management, which recommend implementing limits on collection sizes to prevent denial of service conditions. While requiring user interaction to exploit, this is realistic in normal application usage where power users work with many blueprints throughout the day.
Remediation:
Implement a maximum stack size (recommended 50-100 entries). After pushing new entries in navigateInPageStack, add:
if (pageStack.value.length > MAX_STACK_SIZE) {
pageStack.value.shift();
}This maintains recent navigation history using a FIFO approach while preventing unbounded growth.
References:
- https://cheatsheetseries.owasp.org/cheatsheets/Denial_of_Service_Cheat_Sheet.html
- https://owasp.org/API-Security/editions/2023/en/0xa4-unrestricted-resource-consumption/
- https://web.dev/rail/#response-process-events-in-under-50ms
🔸 Bug (Warning)
| } | ||
|
|
||
| function setActivePageId(componentId: Component["id"]) { | ||
| getComponentAndNavigate(componentId); |
There was a problem hiding this comment.
The function calls getComponentAndNavigate(componentId) before updating activePageId.value on line 915. This causes navigation logic to operate on stale state. When navigateInPageStack calculates the current index by searching for activePageId.value in the stack, it finds the old page ID, not the new one being navigated to. This leads to incorrect decisions about how to modify the stack structure.
Depending on the current stack state, this can cause broken back/forward navigation, duplicate entries, or stack structures that don't reflect actual user navigation paths. The bug produces different incorrect behaviors based on state, making it difficult to debug.
Remediation:
Reorder the function to update state before calling navigation logic:
function setActivePageId(componentId: Component["id"]) {
activePageId.value = componentId;
getComponentAndNavigate(componentId);
}Alternatively, refactor getComponentAndNavigate to accept both old and new page IDs as parameters so it doesn't rely on global state for calculations.
References:
- https://vuejs.org/guide/essentials/reactivity-fundamentals.html
- https://react.dev/learn/you-might-not-need-an-effect
🔸 Bug (Warning)
| c.parentId === "blueprints_root", | ||
| ); | ||
|
|
||
| if (firstBp && firstBp.id !== componentId) { |
There was a problem hiding this comment.
The code accesses firstBp.content.key without optional chaining, which can cause a TypeError if the content property is undefined or null. While the code verifies firstBp exists, it doesn't check that content or key are present. Other parts of this file use the safer pattern component?.content?.key ?? pageId (see lines 952, 964) but this initialization logic is inconsistent.
If a blueprint component has undefined content during initialization or data migration, this will crash the navigation system and prevent users from using the application. This is particularly problematic because it occurs during session startup.
Remediation:
Change line 938 to use optional chaining with a fallback:
key: firstBp.content?.key ?? firstBp.idThis matches the pattern used elsewhere in the navigation stack implementation and ensures the code gracefully handles missing properties.
References:
- https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining
- https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing
🔸 Bug (Warning)
| if (currentIdx + 1 < pageStack.value.length) { | ||
| pageStack.value.splice( | ||
| currentIdx + 1, | ||
| pageStack.value.length - currentIdx - 1, |
There was a problem hiding this comment.
The navigateInPageStack function inconsistently manages activePageId. When called with pageId === null, it directly updates activePageId.value (lines 971, 977). When called with a specific pageId, it only modifies the stack array without updating activePageId, relying on the caller to handle it. This creates an API where the same function sometimes has side effects on global state and sometimes doesn't, depending on parameter values.
This inconsistency makes the code difficult to understand, test, and maintain. When debugging unexpected activePageId values, it's unclear whether the issue is in this function, in setActivePageId, or in their interaction. The mixed responsibility violates the single responsibility principle.
Remediation:
Refactor to make the function consistently update activePageId in all cases, or separate concerns by creating a pure function that calculates stack modifications without side effects. For the first approach, ensure that whenever the stack is modified, activePageId is also updated to match. Alternatively, make navigateInPageStack never update activePageId and have callers handle it explicitly.
References:
- https://martinfowler.com/bliki/CommandQuerySeparation.html
- https://refactoring.guru/smells/alternative-classes-with-different-interfaces
🔸 Bug (Warning)
https://www.loom.com/share/01130b2036bb4c1e96f51514d18ef306
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.