Make memory and context views mobile-friendly#813
Merged
gsxdsm merged 9 commits intoAutoMaker-Org:v1.0.0from Feb 26, 2026
Merged
Make memory and context views mobile-friendly#813gsxdsm merged 9 commits intoAutoMaker-Org:v1.0.0from
gsxdsm merged 9 commits intoAutoMaker-Org:v1.0.0from
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds mobile-responsive behavior and navigation to context and memory views, centralizes filename helpers in image-utils, exposes a header trigger test id, and introduces extensive Playwright E2E tests and test utilities for mobile/desktop flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MobileUI as Mobile UI
participant FileList as File List Panel
participant Content as Content Loader
participant Editor as Editor Panel
User->>MobileUI: Open context/memory view
MobileUI->>FileList: Render file list (full-width if no selection)
User->>FileList: Select file
FileList->>Content: Request file content
Content-->>Editor: Return content
MobileUI->>Editor: Show editor with back button and icon-only toolbar
User->>Editor: Edit / Preview / Save
User->>Editor: Click back button
Editor->>MobileUI: Trigger back navigation
MobileUI->>FileList: Show file list (full-width)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
Summary of ChangesHello @gsxdsm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience for mobile users by making the Memory and Context views fully responsive. It introduces adaptive layouts, mobile-specific navigation, and optimized UI elements to ensure seamless interaction on smaller screens. The changes focus on improving usability and accessibility without compromising functionality, backed by new end-to-end tests to validate the mobile-friendly behavior. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request effectively introduces mobile-friendly responsive layouts for the Memory and Context views. The changes include adaptive layouts, mobile-specific navigation like a back button, and optimized icon-only buttons for smaller screens. The addition of comprehensive end-to-end tests for the new mobile behaviors is a great way to ensure quality. I've found a minor issue regarding code duplication and a subtle bug in the new file extension utility functions, for which I've left a suggestion. Overall, this is a solid improvement to the user experience on mobile devices.
Comment on lines
+63
to
+71
| const isMarkdownFile = (filename: string): boolean => { | ||
| const ext = filename.toLowerCase().substring(filename.lastIndexOf('.')); | ||
| return MARKDOWN_EXTENSIONS.includes(ext); | ||
| }; | ||
|
|
||
| const isImageFile = (filename: string): boolean => { | ||
| const ext = filename.toLowerCase().substring(filename.lastIndexOf('.')); | ||
| return IMAGE_EXTENSIONS.includes(ext); | ||
| }; |
Contributor
There was a problem hiding this comment.
The implementation for getting the file extension has a subtle issue. For a filename without a dot, filename.lastIndexOf('.') returns -1, and substring(-1) on a string returns the whole string. While this works by chance in most cases because the filename is unlikely to be a valid extension, it's not robust. A safer implementation would be to check if a dot exists before getting the substring.
Additionally, these utility functions (isMarkdownFile, isImageFile) and their related constants (MARKDOWN_EXTENSIONS, IMAGE_EXTENSIONS) are duplicated in apps/ui/src/components/views/memory-view.tsx. Consider extracting them to a shared utility file (e.g., in apps/ui/src/lib/) to follow the DRY principle and improve maintainability.
Suggested change
| const isMarkdownFile = (filename: string): boolean => { | |
| const ext = filename.toLowerCase().substring(filename.lastIndexOf('.')); | |
| return MARKDOWN_EXTENSIONS.includes(ext); | |
| }; | |
| const isImageFile = (filename: string): boolean => { | |
| const ext = filename.toLowerCase().substring(filename.lastIndexOf('.')); | |
| return IMAGE_EXTENSIONS.includes(ext); | |
| }; | |
| const isMarkdownFile = (filename: string): boolean => { | |
| const lastDotIndex = filename.lastIndexOf('.'); | |
| if (lastDotIndex < 0) return false; | |
| const ext = filename.substring(lastDotIndex).toLowerCase(); | |
| return MARKDOWN_EXTENSIONS.includes(ext); | |
| }; | |
| const isImageFile = (filename: string): boolean => { | |
| const lastDotIndex = filename.lastIndexOf('.'); | |
| if (lastDotIndex < 0) return false; | |
| const ext = filename.substring(lastDotIndex).toLowerCase(); | |
| return IMAGE_EXTENSIONS.includes(ext); | |
| }; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
apps/ui/tests/memory/mobile-memory-view.spec.ts (1)
56-59: PreferwaitForElementHiddenover rawwaitForFunctionfor dialog close checks.These waits currently require the dialog node to be removed. If UI behavior switches to hidden-but-mounted, tests become flaky. Reusing the existing helper keeps this robust.
💡 Example refactor pattern
- await page.waitForFunction( - () => !document.querySelector('[data-testid="create-memory-dialog"]'), - { timeout: 5000 } - ); + await waitForElementHidden(page, 'create-memory-dialog', { timeout: 5000 });(Apply to each repeated occurrence.)
Also applies to: 99-102, 138-141, 189-192, 243-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/memory/mobile-memory-view.spec.ts` around lines 56 - 59, Replace raw page.waitForFunction checks that assert the dialog node is gone with the existing waitForElementHidden helper to handle hidden-but-mounted dialogs; locate calls that use the selector '[data-testid="create-memory-dialog"]' (and the other repeated selectors at the other occurrences) inside apps/ui/tests/memory/mobile-memory-view.spec.ts and swap each page.waitForFunction(...) invocation to call waitForElementHidden(page, '[data-testid="create-memory-dialog"]', { timeout: 5000 }) (adjust timeout arg to match the original) so tests remain robust when the dialog is hidden but still mounted.apps/ui/tests/context/mobile-context-view.spec.ts (1)
58-61: Use the shared hidden-state helper for dialog dismissal checks.These blocks wait for DOM detachment only. Prefer
waitForElementHiddenso tests pass for both detached and hidden dialog implementations.Also applies to: 100-103, 138-141, 188-191, 241-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/context/mobile-context-view.spec.ts` around lines 58 - 61, Replace direct uses of page.waitForFunction that only check for DOM detachment with the shared waitForElementHidden helper so the tests handle both detached and visually hidden dialogs; specifically, update the call that waits on '[data-testid="create-markdown-dialog"]' (and the other occurrences noted) to call waitForElementHidden with the same selector and timeout options. Locate the occurrences in mobile-context-view.spec.ts where page.waitForFunction is used to assert dialog dismissal (the blocks at the ranges referenced) and swap them to use waitForElementHidden(page, selector, { timeout: 5000 }) so the assertion passes for both detached and hidden implementations. Ensure you import or reference the helper the same way other tests do so there are no missing imports.apps/ui/src/components/views/context-view.tsx (1)
59-71: Consider consolidatingisMarkdownFilewith the existing implementation.There's already an
isMarkdownFileutility inapps/ui/src/components/views/file-editor-view/components/markdown-preview.tsxwith a slightly different implementation (includesmdxextension and handles edge cases differently).Additionally, the current implementation has an edge case: if
filenamehas no.character,lastIndexOf('.')returns-1, makingsubstring(-1)return the last character of the string rather than an empty string.♻️ Suggested fix for edge case handling
const isMarkdownFile = (filename: string): boolean => { - const ext = filename.toLowerCase().substring(filename.lastIndexOf('.')); + const dotIndex = filename.lastIndexOf('.'); + const ext = dotIndex >= 0 ? filename.toLowerCase().substring(dotIndex) : ''; return MARKDOWN_EXTENSIONS.includes(ext); }; const isImageFile = (filename: string): boolean => { - const ext = filename.toLowerCase().substring(filename.lastIndexOf('.')); + const dotIndex = filename.lastIndexOf('.'); + const ext = dotIndex >= 0 ? filename.toLowerCase().substring(dotIndex) : ''; return IMAGE_EXTENSIONS.includes(ext); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/context-view.tsx` around lines 59 - 71, Replace the duplicated isMarkdownFile implementation with the shared utility used in markdown-preview (or export that utility) to avoid divergence; update MARKDOWN_EXTENSIONS to include '.mdx' and adjust isImageFile/isMarkdownFile to safely compute extension by checking filename.lastIndexOf('.') >= 0 before calling substring (or use a reusable getExtension helper) so filenames without a dot return an empty string and don't slice from -1; ensure you reference and use the existing function name isMarkdownFile from markdown-preview (or export it) and keep IMAGE_EXTENSIONS/MARKDOWN_EXTENSIONS constants consistent across modules.apps/ui/src/components/views/memory-view.tsx (1)
46-62: Import the centralizedisMarkdownFileutility instead of duplicating it.The
isMarkdownFilefunction is already exported fromapps/ui/src/components/views/file-editor-view/components/markdown-preview.tsx(line 84). Bothmemory-view.tsxandcontext-view.tsxredefine this function locally; they should import it instead:import { isMarkdownFile } from '@/components/views/file-editor-view/components/markdown-preview';Additionally, the responsive class constants (
FILE_LIST_BASE_CLASSES,EDITOR_PANEL_BASE_CLASSES, etc.) are duplicated verbatim between these two files. Consider extracting them to a shared location like@/lib/responsive-classes.tsif they're reused elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/memory-view.tsx` around lines 46 - 62, Replace the duplicated local isMarkdownFile implementation by importing the centralized function exported from file-editor-view's markdown-preview (use import { isMarkdownFile } from '@/components/views/file-editor-view/components/markdown-preview') and remove MARKDOWN_EXTENSIONS and the local isMarkdownFile definition; also consolidate duplicated responsive class constants (FILE_LIST_BASE_CLASSES, FILE_LIST_DESKTOP_CLASSES, FILE_LIST_EXPANDED_CLASSES, FILE_LIST_MOBILE_NO_SELECTION_CLASSES, FILE_LIST_MOBILE_SELECTION_CLASSES, EDITOR_PANEL_BASE_CLASSES, EDITOR_PANEL_MOBILE_HIDDEN_CLASSES) into a shared module (e.g. export them from '@/lib/responsive-classes' and import those constants here) so both memory-view.tsx and context-view.tsx reuse the same definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ui/src/components/views/context-view.tsx`:
- Around line 251-256: The current handleSelectFile silently drops unsaved edits
by immediately calling loadFileContent(file) and setIsPreviewMode(...); update
handleSelectFile to detect unsaved edits (use the existing unsaved state or
introduce hasUnsavedChanges) and either (a) trigger a mobile-friendly
confirmation flow (e.g., show a toast/confirm component like showDiscardConfirm
that, on user confirm, calls loadFileContent and setIsPreviewMode) or (b)
auto-save the draft first (call saveDraft/currentSaveFunction) and then proceed
to loadFileContent and setIsPreviewMode; ensure you reference handleSelectFile,
loadFileContent, setIsPreviewMode, isMarkdownFile and the unsaved state or save
function when implementing the change.
In `@apps/ui/src/components/views/memory-view.tsx`:
- Around line 150-155: handleSelectFile currently unconditionally calls
loadFileContent and setIsPreviewMode which risks losing unsaved edits; change it
to first check the same unsaved-change indicator used in context-view (e.g., an
isDirty/hasUnsavedChanges state or selector) and, if true on mobile, present the
same confirmation flow (modal/confirm) used in context-view before calling
loadFileContent and setIsPreviewMode; preserve the existing mobile UX branch so
the behavior matches across both views and reference handleSelectFile,
loadFileContent, setIsPreviewMode, and the shared unsaved-change
check/confirmation utility when making the fix.
In `@apps/ui/tests/utils/project/fixtures.ts`:
- Around line 75-77: The helper createMemoryFileOnDisk (and the similar helper
at lines 91-93) currently joins MEMORY_PATH with an unvalidated filename and
allows path traversal; fix by resolving the target path and rejecting any
filename that would escape MEMORY_PATH: compute const target =
path.resolve(MEMORY_PATH, filename) and ensure
target.startsWith(path.resolve(MEMORY_PATH) + path.sep) (or use path.relative
and ensure it doesn't start with '..'); if the check fails throw an error,
otherwise use fs.writeFileSync(target, content).
In `@apps/ui/tests/utils/views/memory.ts`:
- Around line 81-88: The wait in saveMemoryFile currently only inspects
textContent for "Saved" which times out on icon-only mobile buttons; update the
page.waitForFunction used in saveMemoryFile to consider alternative success
indicators on the same element selector '[data-testid="save-memory-file"]' —
e.g., check textContent.includes('Saved') OR
element.getAttribute('aria-label')?.includes('Saved') OR
element.getAttribute('aria-pressed') === 'true' OR the presence of a success
class/disabled attribute (choose the app's reliable indicator) so the wait
succeeds for both text and icon-only buttons.
---
Nitpick comments:
In `@apps/ui/src/components/views/context-view.tsx`:
- Around line 59-71: Replace the duplicated isMarkdownFile implementation with
the shared utility used in markdown-preview (or export that utility) to avoid
divergence; update MARKDOWN_EXTENSIONS to include '.mdx' and adjust
isImageFile/isMarkdownFile to safely compute extension by checking
filename.lastIndexOf('.') >= 0 before calling substring (or use a reusable
getExtension helper) so filenames without a dot return an empty string and don't
slice from -1; ensure you reference and use the existing function name
isMarkdownFile from markdown-preview (or export it) and keep
IMAGE_EXTENSIONS/MARKDOWN_EXTENSIONS constants consistent across modules.
In `@apps/ui/src/components/views/memory-view.tsx`:
- Around line 46-62: Replace the duplicated local isMarkdownFile implementation
by importing the centralized function exported from file-editor-view's
markdown-preview (use import { isMarkdownFile } from
'@/components/views/file-editor-view/components/markdown-preview') and remove
MARKDOWN_EXTENSIONS and the local isMarkdownFile definition; also consolidate
duplicated responsive class constants (FILE_LIST_BASE_CLASSES,
FILE_LIST_DESKTOP_CLASSES, FILE_LIST_EXPANDED_CLASSES,
FILE_LIST_MOBILE_NO_SELECTION_CLASSES, FILE_LIST_MOBILE_SELECTION_CLASSES,
EDITOR_PANEL_BASE_CLASSES, EDITOR_PANEL_MOBILE_HIDDEN_CLASSES) into a shared
module (e.g. export them from '@/lib/responsive-classes' and import those
constants here) so both memory-view.tsx and context-view.tsx reuse the same
definitions.
In `@apps/ui/tests/context/mobile-context-view.spec.ts`:
- Around line 58-61: Replace direct uses of page.waitForFunction that only check
for DOM detachment with the shared waitForElementHidden helper so the tests
handle both detached and visually hidden dialogs; specifically, update the call
that waits on '[data-testid="create-markdown-dialog"]' (and the other
occurrences noted) to call waitForElementHidden with the same selector and
timeout options. Locate the occurrences in mobile-context-view.spec.ts where
page.waitForFunction is used to assert dialog dismissal (the blocks at the
ranges referenced) and swap them to use waitForElementHidden(page, selector, {
timeout: 5000 }) so the assertion passes for both detached and hidden
implementations. Ensure you import or reference the helper the same way other
tests do so there are no missing imports.
In `@apps/ui/tests/memory/mobile-memory-view.spec.ts`:
- Around line 56-59: Replace raw page.waitForFunction checks that assert the
dialog node is gone with the existing waitForElementHidden helper to handle
hidden-but-mounted dialogs; locate calls that use the selector
'[data-testid="create-memory-dialog"]' (and the other repeated selectors at the
other occurrences) inside apps/ui/tests/memory/mobile-memory-view.spec.ts and
swap each page.waitForFunction(...) invocation to call
waitForElementHidden(page, '[data-testid="create-memory-dialog"]', { timeout:
5000 }) (adjust timeout arg to match the original) so tests remain robust when
the dialog is hidden but still mounted.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/ui/src/components/ui/header-actions-panel.tsxapps/ui/src/components/views/context-view.tsxapps/ui/src/components/views/memory-view.tsxapps/ui/tests/context/mobile-context-view.spec.tsapps/ui/tests/memory/mobile-memory-view.spec.tsapps/ui/tests/utils/index.tsapps/ui/tests/utils/navigation/views.tsapps/ui/tests/utils/project/fixtures.tsapps/ui/tests/utils/views/context.tsapps/ui/tests/utils/views/memory.ts
Comment on lines
251
to
256
| const handleSelectFile = (file: ContextFile) => { | ||
| if (hasChanges) { | ||
| // Could add a confirmation dialog here | ||
| } | ||
| // Note: Unsaved changes warning could be added here in the future | ||
| // For now, silently proceed to avoid disrupting mobile UX flow | ||
| loadFileContent(file); | ||
| setIsPreviewMode(isMarkdownFile(file.name)); | ||
| }; |
Contributor
There was a problem hiding this comment.
Potential data loss when switching files with unsaved changes.
The unsaved changes warning was removed to improve mobile UX flow, but this means users could lose edits when selecting a different file. Consider implementing a mobile-friendly confirmation (e.g., a toast with "Discard changes?" or auto-save) rather than silently discarding changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/components/views/context-view.tsx` around lines 251 - 256, The
current handleSelectFile silently drops unsaved edits by immediately calling
loadFileContent(file) and setIsPreviewMode(...); update handleSelectFile to
detect unsaved edits (use the existing unsaved state or introduce
hasUnsavedChanges) and either (a) trigger a mobile-friendly confirmation flow
(e.g., show a toast/confirm component like showDiscardConfirm that, on user
confirm, calls loadFileContent and setIsPreviewMode) or (b) auto-save the draft
first (call saveDraft/currentSaveFunction) and then proceed to loadFileContent
and setIsPreviewMode; ensure you reference handleSelectFile, loadFileContent,
setIsPreviewMode, isMarkdownFile and the unsaved state or save function when
implementing the change.
Comment on lines
150
to
155
| const handleSelectFile = (file: MemoryFile) => { | ||
| if (hasChanges) { | ||
| // Could add a confirmation dialog here | ||
| } | ||
| // Note: Unsaved changes warning could be added here in the future | ||
| // For now, silently proceed to avoid disrupting mobile UX flow | ||
| loadFileContent(file); | ||
| setIsPreviewMode(true); | ||
| }; |
Contributor
There was a problem hiding this comment.
Same unsaved changes concern as context-view.tsx.
The same potential data loss issue applies here. Consider a consistent approach across both views for handling unsaved changes on mobile.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/components/views/memory-view.tsx` around lines 150 - 155,
handleSelectFile currently unconditionally calls loadFileContent and
setIsPreviewMode which risks losing unsaved edits; change it to first check the
same unsaved-change indicator used in context-view (e.g., an
isDirty/hasUnsavedChanges state or selector) and, if true on mobile, present the
same confirmation flow (modal/confirm) used in context-view before calling
loadFileContent and setIsPreviewMode; preserve the existing mobile UX branch so
the behavior matches across both views and reference handleSelectFile,
loadFileContent, setIsPreviewMode, and the shared unsaved-change
check/confirmation utility when making the fix.
Comment on lines
+81
to
+88
| export async function saveMemoryFile(page: Page): Promise<void> { | ||
| await clickElement(page, 'save-memory-file'); | ||
| // Wait for save to complete (button shows "Saved") | ||
| await page.waitForFunction( | ||
| () => | ||
| document.querySelector('[data-testid="save-memory-file"]')?.textContent?.includes('Saved'), | ||
| { timeout: 5000 } | ||
| ); |
Contributor
There was a problem hiding this comment.
saveMemoryFile can fail on mobile due to text-only success check.
At Line 86, the helper only looks for button text containing "Saved". With icon-only mobile actions, this can false-timeout even when save succeeds.
💡 Proposed fix
export async function saveMemoryFile(page: Page): Promise<void> {
await clickElement(page, 'save-memory-file');
- // Wait for save to complete (button shows "Saved")
+ // Wait for save to complete across desktop/mobile variants
await page.waitForFunction(
- () =>
- document.querySelector('[data-testid="save-memory-file"]')?.textContent?.includes('Saved'),
+ () => {
+ const btn = document.querySelector('[data-testid="save-memory-file"]');
+ if (!btn) return false;
+ const stateText = [
+ btn.textContent ?? '',
+ btn.getAttribute('aria-label') ?? '',
+ btn.getAttribute('title') ?? '',
+ ]
+ .join(' ')
+ .toLowerCase();
+ return stateText.includes('saved');
+ },
{ timeout: 5000 }
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/tests/utils/views/memory.ts` around lines 81 - 88, The wait in
saveMemoryFile currently only inspects textContent for "Saved" which times out
on icon-only mobile buttons; update the page.waitForFunction used in
saveMemoryFile to consider alternative success indicators on the same element
selector '[data-testid="save-memory-file"]' — e.g., check
textContent.includes('Saved') OR
element.getAttribute('aria-label')?.includes('Saved') OR
element.getAttribute('aria-pressed') === 'true' OR the presence of a success
class/disabled attribute (choose the app's reliable indicator) so the wait
succeeds for both text and icon-only buttons.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/ui/src/components/views/memory-view.tsx (1)
152-157:⚠️ Potential issue | 🟠 MajorUnsaved edits can be discarded on file switch and mobile back navigation.
Both navigation paths proceed without checking
hasChanges, so in-progress edits can be lost silently.Suggested fix
+ const confirmDiscardChanges = (): boolean => { + if (!hasChanges) return true; + return window.confirm('Discard unsaved changes?'); + }; const handleSelectFile = (file: MemoryFile) => { - // Note: Unsaved changes warning could be added here in the future - // For now, silently proceed to avoid disrupting mobile UX flow + if (!confirmDiscardChanges()) return; loadFileContent(file); setIsPreviewMode(true); };- <Button + <Button variant="ghost" size="sm" - onClick={() => setSelectedFile(null)} + onClick={() => { + if (!confirmDiscardChanges()) return; + setSelectedFile(null); + }} className="shrink-0 -ml-1"Also applies to: 500-505
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/memory-view.tsx` around lines 152 - 157, handleSelectFile currently calls loadFileContent(file) and setIsPreviewMode(true) unconditionally, which discards in-progress edits tracked by hasChanges; update handleSelectFile to check the hasChanges flag and, if true, open a confirmation/save dialog (or prompt) instead of immediately switching, and only call loadFileContent(file) and setIsPreviewMode(true) after the user confirms/discards/save; apply the same guard to the mobile/back navigation handler (e.g., onBackClick or the function handling navigation) so both file switching and back navigation respect hasChanges and invoke the same confirmation/save flow.apps/ui/src/components/views/context-view.tsx (1)
255-260:⚠️ Potential issue | 🟠 MajorUnsaved edits can still be lost on file change and mobile back.
Navigation currently proceeds even when
hasChangesis true, so users can lose uncommitted edits.Suggested fix
+ const confirmDiscardChanges = (): boolean => { + if (!hasChanges) return true; + return window.confirm('Discard unsaved changes?'); + }; const handleSelectFile = (file: ContextFile) => { - // Note: Unsaved changes warning could be added here in the future - // For now, silently proceed to avoid disrupting mobile UX flow + if (!confirmDiscardChanges()) return; loadFileContent(file); setIsPreviewMode(isMarkdownFile(file.name)); };- <Button + <Button variant="ghost" size="sm" - onClick={() => setSelectedFile(null)} + onClick={() => { + if (!confirmDiscardChanges()) return; + setSelectedFile(null); + }} className="shrink-0 -ml-1"Also applies to: 927-932
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/context-view.tsx` around lines 255 - 260, handleSelectFile currently always switches files and can discard uncommitted edits; update it to check the existing hasChanges flag before calling loadFileContent and setIsPreviewMode. If hasChanges is true, present a confirmation (or save) flow — e.g., show a confirm/discard modal or trigger a save action and wait for completion — and only call loadFileContent(file) and setIsPreviewMode(isMarkdownFile(file.name)) after the user confirms discard or after a successful save. Implement the same guard-and-confirm behavior for the other file-change handler referenced around lines 927-932 so both paths respect hasChanges.
🧹 Nitpick comments (9)
apps/ui/tests/context/mobile-context-view.spec.ts (1)
199-209: Icon visibility check can be simplified.The IIFE pattern for checking icon visibility is verbose. Playwright's
or()locator can simplify this.♻️ Simplified approach
- // Button should have icon (Eye or Pencil) - const eyeIcon = toggleButton.locator('svg.lucide-eye'); - const pencilIcon = toggleButton.locator('svg.lucide-pencil'); - - // One of the icons should be present - const hasIcon = await (async () => { - const eyeVisible = await eyeIcon.isVisible().catch(() => false); - const pencilVisible = await pencilIcon.isVisible().catch(() => false); - return eyeVisible || pencilVisible; - })(); - - expect(hasIcon).toBe(true); + // Button should have icon (Eye or Pencil) + const icon = toggleButton.locator('svg.lucide-eye, svg.lucide-pencil'); + await expect(icon).toBeVisible();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/context/mobile-context-view.spec.ts` around lines 199 - 209, Replace the verbose IIFE visibility check with Playwright's locator.or() to combine the two icon locators into one; locate the combined icon from toggleButton (using eyeIcon.or(pencilIcon) or toggleButton.locator('svg.lucide-eye').or(toggleButton.locator('svg.lucide-pencil'))), then call isVisible() (or check count()>0) on that single locator and assert it is true, replacing the async IIFE that sets hasIcon and the subsequent expect.apps/ui/tests/context/mobile-context-operations.spec.ts (1)
97-105: Hover may not work reliably on mobile touch devices.Line 99 uses
hover()on a mobile viewport (iPhone 13 Pro), but touch devices don't have hover states. The test may pass because Playwright simulates mouse events, but this doesn't reflect real mobile user behavior. Consider usingtap()orclick()with appropriate waits instead.Additionally,
{ force: true }at line 102 bypasses Playwright's actionability checks, which could mask real issues where the button isn't properly visible or clickable.♻️ Suggested improvement
- // Click on the file menu dropdown - hover first to make it visible - const fileRow = page.locator(`[data-testid="context-file-${fileName}"]`); - await fileRow.hover(); - - const fileMenuButton = page.locator(`[data-testid="context-file-menu-${fileName}"]`); - await fileMenuButton.click({ force: true }); - - // Wait for dropdown - await page.waitForTimeout(300); + // On mobile, the menu button should be visible without hover + const fileMenuButton = page.locator(`[data-testid="context-file-menu-${fileName}"]`); + await fileMenuButton.waitFor({ state: 'visible', timeout: 5000 }); + await fileMenuButton.click(); + + // Wait for dropdown menu to appear + await page.waitForSelector('[role="menu"]', { state: 'visible', timeout: 3000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/context/mobile-context-operations.spec.ts` around lines 97 - 105, The test uses fileRow.hover() and fileMenuButton.click({ force: true }) which are unreliable on mobile/touch viewports; replace hover() with a touch-friendly action such as page.tap() or fileRow.click() to reveal the menu, remove the force:true bypass, and instead wait for the menu button to become visible/actionable (e.g., use fileMenuButton.waitFor({ state: "visible" }) or an equivalent wait) before clicking; update the sequence around the fileRow, fileMenuButton and the explicit page.waitForTimeout(300) to use deterministic waits rather than hover/force so the test simulates real mobile interactions.apps/ui/tests/utils/views/memory.ts (2)
17-20:clickMemoryFilemay be flaky without visibility wait.Unlike
clickElementfrom the core interactions module (which waits for visibility before clicking), this helper directly clicks without waiting. This could cause flaky tests if the file button isn't immediately visible.♻️ Proposed fix
export async function clickMemoryFile(page: Page, fileName: string): Promise<void> { const fileButton = page.locator(`[data-testid="memory-file-${fileName}"]`); + await fileButton.waitFor({ state: 'visible', timeout: 10000 }); await fileButton.click(); }Alternatively, reuse the existing
clickElementhelper:+import { clickElement } from '../core/interactions'; + export async function clickMemoryFile(page: Page, fileName: string): Promise<void> { - const fileButton = page.locator(`[data-testid="memory-file-${fileName}"]`); - await fileButton.click(); + await clickElement(page, `memory-file-${fileName}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/utils/views/memory.ts` around lines 17 - 20, The helper clickMemoryFile directly clicks the locator page.locator(`[data-testid="memory-file-${fileName}"]`) and can be flaky; modify clickMemoryFile to wait for visibility before clicking by either calling the existing clickElement helper (reuse clickElement(page, `[data-testid="memory-file-${fileName}"]`) ) or awaiting the locator to be visible (e.g., await fileButton.waitFor({ state: "visible" })) prior to calling fileButton.click(); keep the same function signature.
189-191: ArbitrarywaitForTimeoutis a test smell.Hardcoded timeouts like
waitForTimeout(100)can cause flakiness—either too short (race condition) or too long (slow tests). Consider removing it or replacing with a condition-based wait if there's a specific state to wait for.💡 Suggestion
export async function navigateToMemory(page: Page): Promise<void> { // Wait for any pending navigation to complete before starting a new one await page.waitForLoadState('domcontentloaded').catch(() => {}); - await page.waitForTimeout(100); // Navigate directly to /memory route await page.goto('/memory', { waitUntil: 'domcontentloaded' });If the timeout addresses a specific race condition, document it or use a more robust wait.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/utils/views/memory.ts` around lines 189 - 191, The hardcoded page.waitForTimeout(100) in apps/ui/tests/utils/views/memory.ts is a flaky arbitrary delay; remove it and replace with a deterministic, condition-based wait tied to the actual thing you need (e.g., await page.waitForLoadState('networkidle'), await page.waitForResponse(...), or await page.waitForSelector(...) depending on the race), or at minimum document why the short sleep is required; update the logic around the existing await page.waitForLoadState('domcontentloaded') so the test waits for the concrete state instead of a fixed timeout.apps/ui/tests/utils/project/fixtures.ts (1)
67-70: Context file operations lack path traversal protection.The new memory file operations use
resolveMemoryFixturePathto guard against path traversal, butcreateContextFileOnDiskstill uses unvalidatedpath.join(CONTEXT_PATH, filename). For consistency and defense-in-depth, consider adding similar protection to context operations.♻️ Proposed fix to add path validation for context operations
+/** + * Resolve and validate a context fixture path to prevent path traversal + */ +function resolveContextFixturePath(filename: string): string { + const resolved = path.resolve(CONTEXT_PATH, filename); + const base = path.resolve(CONTEXT_PATH) + path.sep; + if (!resolved.startsWith(base)) { + throw new Error(`Invalid context filename: ${filename}`); + } + return resolved; +} + /** * Create a context file directly on disk (for test setup) */ export function createContextFileOnDisk(filename: string, content: string): void { - const filePath = path.join(CONTEXT_PATH, filename); + const filePath = resolveContextFixturePath(filename); fs.writeFileSync(filePath, content); }Also apply the same fix to
contextFileExistsOnDiskat lines 95-98.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/utils/project/fixtures.ts` around lines 67 - 70, createContextFileOnDisk currently constructs filePath with path.join(CONTEXT_PATH, filename) and allows path traversal; validate and normalize the filename first by reusing the existing resolveMemoryFixturePath (or a similar sanitizer) to produce a safe path inside CONTEXT_PATH, then use that validated path in fs.writeFileSync. Do the same validation for contextFileExistsOnDisk (replace direct path.join/CALL to fs.existsSync with a call that uses the resolved/validated path) so both functions consistently guard against path traversal.apps/ui/tests/context/file-extension-edge-cases.spec.ts (1)
132-134: Arbitrary timeout - consider condition-based wait.
waitForTimeout(1000)is a test smell. Consider waiting for a specific condition like the file list loading complete or using retry assertions.💡 Suggestion
- // Wait a moment for files to load - await page.waitForTimeout(1000); + // Wait for file list to be visible and stable + const fileList = page.locator('[data-testid="context-file-list"]'); + await expect(fileList).toBeVisible();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/context/file-extension-edge-cases.spec.ts` around lines 132 - 134, The test uses an arbitrary sleep via page.waitForTimeout(1000); replace this with a condition-based wait: locate the file list or loading indicator used in this spec (e.g., the DOM node representing the file list or a loading spinner) and wait for it to reach the expected state using Playwright APIs such as page.waitForSelector(...) or expect(locator).toHaveCount(...) / expect(locator).toBeHidden() instead of the fixed timeout; update the test around page.waitForTimeout to wait for the file list to appear/populate or for the loading indicator to disappear so the test becomes deterministic.apps/ui/tests/memory/mobile-memory-operations.spec.ts (1)
97-105: Same hover/force-click pattern concerns as context operations.This has the same issues flagged in
mobile-context-operations.spec.ts: hover may not work reliably on touch devices, andforce: truebypasses actionability checks. Consider the same refactoring approach.♻️ See mobile-context-operations.spec.ts for suggested fix pattern
- // Click on the file menu dropdown - hover first to make it visible - const fileRow = page.locator(`[data-testid="memory-file-${fileName}"]`); - await fileRow.hover(); - - const fileMenuButton = page.locator(`[data-testid="memory-file-menu-${fileName}"]`); - await fileMenuButton.click({ force: true }); - - // Wait for dropdown - await page.waitForTimeout(300); + // On mobile, the menu button should be visible without hover + const fileMenuButton = page.locator(`[data-testid="memory-file-menu-${fileName}"]`); + await fileMenuButton.waitFor({ state: 'visible', timeout: 5000 }); + await fileMenuButton.click(); + + // Wait for dropdown menu to appear + await page.waitForSelector('[role="menu"]', { state: 'visible', timeout: 3000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/memory/mobile-memory-operations.spec.ts` around lines 97 - 105, Replace the unreliable hover + force-click pattern: remove the fileRow.hover() call and the fileMenuButton.click({ force: true }) call, instead wait for the actual menu toggle to be visible/actionable and click it normally; e.g., use the existing locators fileRow and fileMenuButton (or locate a specific toggle within fileRow like `[data-testid="memory-file-menu-${fileName}"]`), call await fileMenuButton.waitFor({ state: 'visible' }) (or await fileRow.waitFor(...)) and then await fileMenuButton.click() so actionability checks run and the test works on touch/hover-less environments without artificial force clicks or timeouts.apps/ui/tests/memory/mobile-memory-view.spec.ts (1)
199-205: Same icon visibility simplification applies here.This uses the same verbose IIFE pattern as
mobile-context-view.spec.ts. The simplified approach using comma-separated selectors would work here too.♻️ Simplified approach
- // Button should have icon (Eye or Pencil) - const eyeIcon = toggleButton.locator('svg.lucide-eye'); - const pencilIcon = toggleButton.locator('svg.lucide-pencil'); - - // One of the icons should be present - const hasIcon = await (async () => { - const eyeVisible = await eyeIcon.isVisible().catch(() => false); - const pencilVisible = await pencilIcon.isVisible().catch(() => false); - return eyeVisible || pencilVisible; - })(); - - expect(hasIcon).toBe(true); + // Button should have icon (Eye or Pencil) + const icon = toggleButton.locator('svg.lucide-eye, svg.lucide-pencil'); + await expect(icon).toBeVisible();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/memory/mobile-memory-view.spec.ts` around lines 199 - 205, The current verbose IIFE that computes hasIcon by calling eyeIcon.isVisible().catch(...) and pencilIcon.isVisible().catch(...) should be replaced with a single combined selector check: create a locator that targets both icons via a comma-separated selector (replace the separate eyeIcon/pencilIcon checks) and await its visibility (e.g., using locator('eyeSelector, pencilSelector').first().isVisible() or locator.count() > 0 combined with isVisible) to set hasIcon; update the code that references hasIcon accordingly so you no longer use the IIFE or per-icon .catch() calls.apps/ui/tests/utils/project/fixtures.spec.ts (1)
73-84: Consider clarifying the nested path test's scope.The test verifies that
memoryFileExistsOnDiskdoesn't throw for nested paths, but the comment suggests uncertainty about whether subdirectories are supported. Consider adding a follow-up test that verifies the actual creation behavior (whether it succeeds or fails with a clear error when the parent directory doesn't exist).💡 Suggested enhancement
test('should accept nested paths within memory directory', () => { // Note: This tests the boundary - if subdirectories are supported, // this should pass; if not, it should throw const nestedFilename = 'subfolder/nested-file.md'; // Currently, the implementation doesn't create subdirectories, // so this would fail when trying to write. But the path itself // is valid (doesn't escape the memory directory) expect(() => { memoryFileExistsOnDisk(nestedFilename); }).not.toThrow(); + + // Verify creation fails gracefully when parent directory doesn't exist + expect(() => { + createMemoryFileOnDisk(nestedFilename, 'content'); + }).toThrow(); // ENOENT expected });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/utils/project/fixtures.spec.ts` around lines 73 - 84, The current test "should accept nested paths within memory directory" using memoryFileExistsOnDisk and nestedFilename is ambiguous about whether subdirectories are actually created; add a concise follow-up test that explicitly attempts to create/write the same nested path (use the project helper method that performs writes—e.g., the write/create method used elsewhere in tests) and then assert the expected behavior (either that the file was created and is readable via memoryFileExistsOnDisk, or that the write throws a specific, documented error), and update the test comment to state which behavior your implementation guarantees so the intent is clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ui/tests/context/file-extension-edge-cases.spec.ts`:
- Around line 160-168: The test currently swallows assertion failures by
attaching .catch(() => {}) to the expect(...).not.toBeVisible call; locate the
fileButton locator created with
page.locator(`[data-testid="context-file-${fileName}"]`) and remove the trailing
.catch so the expect(fileButton).not.toBeVisible({ timeout: 3000 }) assertion
can fail properly; if the behavior is genuinely uncertain, replace the silent
catch with explicit branching or a documented assertion (e.g., try the expect
and on failure assert the alternative expected outcome or add a clear comment)
so the test outcome is deterministic and not silently ignored.
In `@apps/ui/tests/memory/file-extension-edge-cases.spec.ts`:
- Around line 162-166: The test currently swallows assertion failures by
chaining .catch(() => {}) onto await expect(fileButton).not.toBeVisible({
timeout: 3000 }); — remove the .catch and let the expectation fail the test if
fileButton is visible, i.e. call await expect(fileButton).not.toBeVisible({
timeout: 3000 }); directly (or if you need conditional behavior, wrap in a
try/catch that rethrows on failure after inspecting the error) to ensure the
assertion is enforced.
---
Duplicate comments:
In `@apps/ui/src/components/views/context-view.tsx`:
- Around line 255-260: handleSelectFile currently always switches files and can
discard uncommitted edits; update it to check the existing hasChanges flag
before calling loadFileContent and setIsPreviewMode. If hasChanges is true,
present a confirmation (or save) flow — e.g., show a confirm/discard modal or
trigger a save action and wait for completion — and only call
loadFileContent(file) and setIsPreviewMode(isMarkdownFile(file.name)) after the
user confirms discard or after a successful save. Implement the same
guard-and-confirm behavior for the other file-change handler referenced around
lines 927-932 so both paths respect hasChanges.
In `@apps/ui/src/components/views/memory-view.tsx`:
- Around line 152-157: handleSelectFile currently calls loadFileContent(file)
and setIsPreviewMode(true) unconditionally, which discards in-progress edits
tracked by hasChanges; update handleSelectFile to check the hasChanges flag and,
if true, open a confirmation/save dialog (or prompt) instead of immediately
switching, and only call loadFileContent(file) and setIsPreviewMode(true) after
the user confirms/discards/save; apply the same guard to the mobile/back
navigation handler (e.g., onBackClick or the function handling navigation) so
both file switching and back navigation respect hasChanges and invoke the same
confirmation/save flow.
---
Nitpick comments:
In `@apps/ui/tests/context/file-extension-edge-cases.spec.ts`:
- Around line 132-134: The test uses an arbitrary sleep via
page.waitForTimeout(1000); replace this with a condition-based wait: locate the
file list or loading indicator used in this spec (e.g., the DOM node
representing the file list or a loading spinner) and wait for it to reach the
expected state using Playwright APIs such as page.waitForSelector(...) or
expect(locator).toHaveCount(...) / expect(locator).toBeHidden() instead of the
fixed timeout; update the test around page.waitForTimeout to wait for the file
list to appear/populate or for the loading indicator to disappear so the test
becomes deterministic.
In `@apps/ui/tests/context/mobile-context-operations.spec.ts`:
- Around line 97-105: The test uses fileRow.hover() and fileMenuButton.click({
force: true }) which are unreliable on mobile/touch viewports; replace hover()
with a touch-friendly action such as page.tap() or fileRow.click() to reveal the
menu, remove the force:true bypass, and instead wait for the menu button to
become visible/actionable (e.g., use fileMenuButton.waitFor({ state: "visible"
}) or an equivalent wait) before clicking; update the sequence around the
fileRow, fileMenuButton and the explicit page.waitForTimeout(300) to use
deterministic waits rather than hover/force so the test simulates real mobile
interactions.
In `@apps/ui/tests/context/mobile-context-view.spec.ts`:
- Around line 199-209: Replace the verbose IIFE visibility check with
Playwright's locator.or() to combine the two icon locators into one; locate the
combined icon from toggleButton (using eyeIcon.or(pencilIcon) or
toggleButton.locator('svg.lucide-eye').or(toggleButton.locator('svg.lucide-pencil'))),
then call isVisible() (or check count()>0) on that single locator and assert it
is true, replacing the async IIFE that sets hasIcon and the subsequent expect.
In `@apps/ui/tests/memory/mobile-memory-operations.spec.ts`:
- Around line 97-105: Replace the unreliable hover + force-click pattern: remove
the fileRow.hover() call and the fileMenuButton.click({ force: true }) call,
instead wait for the actual menu toggle to be visible/actionable and click it
normally; e.g., use the existing locators fileRow and fileMenuButton (or locate
a specific toggle within fileRow like
`[data-testid="memory-file-menu-${fileName}"]`), call await
fileMenuButton.waitFor({ state: 'visible' }) (or await fileRow.waitFor(...)) and
then await fileMenuButton.click() so actionability checks run and the test works
on touch/hover-less environments without artificial force clicks or timeouts.
In `@apps/ui/tests/memory/mobile-memory-view.spec.ts`:
- Around line 199-205: The current verbose IIFE that computes hasIcon by calling
eyeIcon.isVisible().catch(...) and pencilIcon.isVisible().catch(...) should be
replaced with a single combined selector check: create a locator that targets
both icons via a comma-separated selector (replace the separate
eyeIcon/pencilIcon checks) and await its visibility (e.g., using
locator('eyeSelector, pencilSelector').first().isVisible() or locator.count() >
0 combined with isVisible) to set hasIcon; update the code that references
hasIcon accordingly so you no longer use the IIFE or per-icon .catch() calls.
In `@apps/ui/tests/utils/project/fixtures.spec.ts`:
- Around line 73-84: The current test "should accept nested paths within memory
directory" using memoryFileExistsOnDisk and nestedFilename is ambiguous about
whether subdirectories are actually created; add a concise follow-up test that
explicitly attempts to create/write the same nested path (use the project helper
method that performs writes—e.g., the write/create method used elsewhere in
tests) and then assert the expected behavior (either that the file was created
and is readable via memoryFileExistsOnDisk, or that the write throws a specific,
documented error), and update the test comment to state which behavior your
implementation guarantees so the intent is clear.
In `@apps/ui/tests/utils/project/fixtures.ts`:
- Around line 67-70: createContextFileOnDisk currently constructs filePath with
path.join(CONTEXT_PATH, filename) and allows path traversal; validate and
normalize the filename first by reusing the existing resolveMemoryFixturePath
(or a similar sanitizer) to produce a safe path inside CONTEXT_PATH, then use
that validated path in fs.writeFileSync. Do the same validation for
contextFileExistsOnDisk (replace direct path.join/CALL to fs.existsSync with a
call that uses the resolved/validated path) so both functions consistently guard
against path traversal.
In `@apps/ui/tests/utils/views/memory.ts`:
- Around line 17-20: The helper clickMemoryFile directly clicks the locator
page.locator(`[data-testid="memory-file-${fileName}"]`) and can be flaky; modify
clickMemoryFile to wait for visibility before clicking by either calling the
existing clickElement helper (reuse clickElement(page,
`[data-testid="memory-file-${fileName}"]`) ) or awaiting the locator to be
visible (e.g., await fileButton.waitFor({ state: "visible" })) prior to calling
fileButton.click(); keep the same function signature.
- Around line 189-191: The hardcoded page.waitForTimeout(100) in
apps/ui/tests/utils/views/memory.ts is a flaky arbitrary delay; remove it and
replace with a deterministic, condition-based wait tied to the actual thing you
need (e.g., await page.waitForLoadState('networkidle'), await
page.waitForResponse(...), or await page.waitForSelector(...) depending on the
race), or at minimum document why the short sleep is required; update the logic
around the existing await page.waitForLoadState('domcontentloaded') so the test
waits for the concrete state instead of a fixed timeout.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/ui/src/components/views/context-view.tsxapps/ui/src/components/views/memory-view.tsxapps/ui/tests/context/desktop-context-view.spec.tsapps/ui/tests/context/file-extension-edge-cases.spec.tsapps/ui/tests/context/mobile-context-operations.spec.tsapps/ui/tests/context/mobile-context-view.spec.tsapps/ui/tests/memory/desktop-memory-view.spec.tsapps/ui/tests/memory/file-extension-edge-cases.spec.tsapps/ui/tests/memory/mobile-memory-operations.spec.tsapps/ui/tests/memory/mobile-memory-view.spec.tsapps/ui/tests/utils/project/fixtures.spec.tsapps/ui/tests/utils/project/fixtures.tsapps/ui/tests/utils/views/context.tsapps/ui/tests/utils/views/memory.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/ui/tests/utils/views/context.ts
Comment on lines
+160
to
+168
| // File should NOT appear in list because UI enforces .md extension | ||
| // (The UI may add .md automatically or show validation error) | ||
| const fileButton = page.locator(`[data-testid="context-file-${fileName}"]`); | ||
| await expect(fileButton) | ||
| .not.toBeVisible({ timeout: 3000 }) | ||
| .catch(() => { | ||
| // It's OK if it doesn't appear - that's expected behavior | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Test assertion is swallowed, making it ineffective.
The .catch(() => {}) block causes the test to always pass regardless of whether the file appears or not. If the intent is to verify the file doesn't appear, remove the catch block. If the behavior is uncertain, the test should document the expected behavior clearly.
🐛 Proposed fix
// File should NOT appear in list because UI enforces .md extension
// (The UI may add .md automatically or show validation error)
const fileButton = page.locator(`[data-testid="context-file-${fileName}"]`);
- await expect(fileButton)
- .not.toBeVisible({ timeout: 3000 })
- .catch(() => {
- // It's OK if it doesn't appear - that's expected behavior
- });
+ // If UI auto-adds .md extension, check for that instead
+ const fileWithExtension = page.locator(`[data-testid="context-file-${fileName}.md"]`);
+ const eitherVisible = await fileButton.or(fileWithExtension).isVisible().catch(() => false);
+
+ // Either the file without extension should not appear,
+ // or it should appear with .md auto-added
+ if (!eitherVisible) {
+ await expect(fileButton).not.toBeVisible({ timeout: 3000 });
+ } else {
+ // UI auto-added .md extension - verify that behavior
+ await expect(fileWithExtension).toBeVisible();
+ }Alternatively, if the expected behavior is clear:
- await expect(fileButton)
- .not.toBeVisible({ timeout: 3000 })
- .catch(() => {
- // It's OK if it doesn't appear - that's expected behavior
- });
+ await expect(fileButton).not.toBeVisible({ timeout: 3000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/tests/context/file-extension-edge-cases.spec.ts` around lines 160 -
168, The test currently swallows assertion failures by attaching .catch(() =>
{}) to the expect(...).not.toBeVisible call; locate the fileButton locator
created with page.locator(`[data-testid="context-file-${fileName}"]`) and remove
the trailing .catch so the expect(fileButton).not.toBeVisible({ timeout: 3000 })
assertion can fail properly; if the behavior is genuinely uncertain, replace the
silent catch with explicit branching or a documented assertion (e.g., try the
expect and on failure assert the alternative expected outcome or add a clear
comment) so the test outcome is deterministic and not silently ignored.
Comment on lines
+162
to
+166
| await expect(fileButton) | ||
| .not.toBeVisible({ timeout: 3000 }) | ||
| .catch(() => { | ||
| // It's OK if it doesn't appear - that's expected behavior | ||
| }); |
Contributor
There was a problem hiding this comment.
Assertion is swallowed, so this test can pass on regressions.
The .catch(() => {}) suppresses failures from not.toBeVisible, which makes this check non-enforcing.
Suggested fix
- await expect(fileButton)
- .not.toBeVisible({ timeout: 3000 })
- .catch(() => {
- // It's OK if it doesn't appear - that's expected behavior
- });
+ await expect(fileButton).toHaveCount(0);
+ // If auto-append is the intended contract, assert that explicitly:
+ await expect(page.locator(`[data-testid="memory-file-${fileName}.md"]`)).toBeVisible();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/tests/memory/file-extension-edge-cases.spec.ts` around lines 162 -
166, The test currently swallows assertion failures by chaining .catch(() => {})
onto await expect(fileButton).not.toBeVisible({ timeout: 3000 }); — remove the
.catch and let the expectation fail the test if fileButton is visible, i.e. call
await expect(fileButton).not.toBeVisible({ timeout: 3000 }); directly (or if you
need conditional behavior, wrap in a try/catch that rethrows on failure after
inspecting the error) to ensure the assertion is enforced.
Code review improvements: - Extract isMarkdownFilename and isImageFilename to shared image-utils.ts - Remove duplicated code from context-view.tsx and memory-view.tsx - Add path traversal guard for context fixture utilities (matching memory) - Add 7 new tests for context fixture path traversal protection - Total 61 tests pass Addresses code review feedback from PR AutoMaker-Org#813 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
apps/ui/src/components/views/context-view.tsx (1)
237-242:⚠️ Potential issue | 🟠 MajorUnsaved edits can still be lost when switching files.
Line 240 immediately loads the newly selected file even when
hasChangesis true, so in-progress edits can be discarded silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/context-view.tsx` around lines 237 - 242, handleSelectFile currently calls loadFileContent(file) unconditionally which discards in-progress edits when hasChanges is true; update handleSelectFile to check the hasChanges flag and, if true, show a confirmation/save prompt (e.g., call an existing promptSaveChanges/confirmDiscard helper or open a modal) and only call loadFileContent(file) and setIsPreviewMode(...) after the user either saves or explicitly confirms discarding changes; if the user cancels, abort and keep the current file and edit state.apps/ui/src/components/views/memory-view.tsx (1)
143-148:⚠️ Potential issue | 🟠 MajorUnsaved edits can still be discarded on file switch.
handleSelectFiledirectly loads the next file without checkinghasChanges, so current edits can be lost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/memory-view.tsx` around lines 143 - 148, handleSelectFile currently calls loadFileContent(file) and setIsPreviewMode(true) unconditionally which allows unsaved edits (hasChanges) to be discarded; update handleSelectFile to check hasChanges first and, if true, show a confirmation/save prompt (or call the existing save routine) and only proceed to call loadFileContent(file) and setIsPreviewMode(true) when the user confirms discard or after a successful save; reference the handleSelectFile function and the hasChanges flag, and wire the modal/confirmation's onConfirm to perform loadFileContent(file) and setIsPreviewMode(true).
🧹 Nitpick comments (4)
apps/ui/tests/projects/board-background-persistence.spec.ts (3)
187-189: Fixed timeouts may cause flakiness.Using
waitForTimeout(2000)for settings load synchronization is a common workaround when there's no observable event to wait on. However, this can cause flakiness on slower CI runners or fail to wait long enough on resource-constrained environments.Consider whether there's a deterministic signal (e.g., a DOM attribute, network request completion, or state indicator) that could replace or supplement these fixed waits in the future.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/projects/board-background-persistence.spec.ts` around lines 187 - 189, The fixed 2s sleep using page.waitForTimeout(2000) (in the test that relies on useProjectSettingsLoader) is flaky; replace it with a deterministic wait such as page.waitForResponse(...) for the settings API request, or page.waitForSelector(...) / page.waitForFunction(...) that checks a DOM attribute or global state indicating settings loaded (e.g., a response URL match for the settings endpoint or a data-testid like "project-settings-loaded" or window.__projectSettingsLoaded). Update the test to await that signal instead of page.waitForTimeout and keep the reference to page.waitForTimeout and useProjectSettingsLoader so reviewers can find the change.
73-76: Add a guard or clearer error for missing test fixture image.The test assumes a background image exists at
../img/background.jpg. If this file is missing (e.g., not committed, deleted, or path changed),fs.copyFileSyncwill throw a genericENOENTerror that doesn't clearly indicate the test fixture is missing.💡 Optional: Add existence check with descriptive error
// Copy actual background image from test fixtures const backgroundPath = path.join(automakerDirA, 'board', 'background.jpg'); const testImagePath = path.join(__dirname, '..', 'img', 'background.jpg'); + if (!fs.existsSync(testImagePath)) { + throw new Error(`Test fixture image not found at ${testImagePath}. Ensure apps/ui/tests/img/background.jpg exists.`); + } fs.copyFileSync(testImagePath, backgroundPath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/projects/board-background-persistence.spec.ts` around lines 73 - 76, Add a guard around the test fixture copy so missing fixture errors are clear: before calling fs.copyFileSync with testImagePath and backgroundPath (the variables named testImagePath and backgroundPath in this test) check for the file's existence (e.g., fs.existsSync(testImagePath)) and if it's absent throw or fail the test with a descriptive message like "missing test fixture ../img/background.jpg" or wrap fs.copyFileSync in a try/catch and rethrow a new Error that includes that descriptive message plus the original error; this ensures failures from fs.copyFileSync are explicit about the missing fixture.
319-361: UseSTORE_VERSIONSconstants instead of hardcoding version numbers.The test hardcodes
version: 2(line 342) andversion: 1(line 353) in localStorage setup. ThesetupRealProjecthelper function uses centralizedSTORE_VERSIONSconstants (versions.APP_STOREandversions.SETUP_STORE), making it maintainable if versions ever change. Either exportSTORE_VERSIONSfrom the setup utilities and import it, or refactor the test to use thesetupRealProjecthelper function which already handles correct versioning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/projects/board-background-persistence.spec.ts` around lines 319 - 361, The test currently hardcodes store version numbers (version: 2 and version: 1) when seeding localStorage in the addInitScript; replace those literals with the centralized store version constants (e.g., STORE_VERSIONS or the existing versions.APP_STORE and versions.SETUP_STORE) or call the existing setupRealProject helper which already uses the correct versions; locate the localStorage setup inside the addInitScript and swap the numeric literals for the appropriate constants (or refactor to use setupRealProject) so the test stays in sync with STORE_VERSIONS.apps/ui/tests/utils/project/fixtures.ts (1)
80-82: Create parent directories before writing fixture files.If tests pass nested relative paths (e.g.,
nested/a.md), these helpers can fail withENOENTunless the parent directory exists.💡 Suggested fix
export function createContextFileOnDisk(filename: string, content: string): void { const filePath = resolveContextFixturePath(filename); + fs.mkdirSync(path.dirname(filePath), { recursive: true }); fs.writeFileSync(filePath, content); } @@ export function createMemoryFileOnDisk(filename: string, content: string): void { const filePath = resolveMemoryFixturePath(filename); + fs.mkdirSync(path.dirname(filePath), { recursive: true }); fs.writeFileSync(filePath, content); }Also applies to: 100-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/utils/project/fixtures.ts` around lines 80 - 82, The helper that writes fixture files uses resolveContextFixturePath(filename) and then fs.writeFileSync(filePath, content) but doesn't ensure parent directories exist, causing ENOENT for nested paths; before calling fs.writeFileSync in the fixture writer (and the similar writer around lines 100-101), create the parent directory with something like fs.mkdirSync(path.dirname(filePath), { recursive: true }) so the directory tree exists, then proceed to write the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ui/playwright.config.ts`:
- Around line 62-63: The current Playwright config blindly sets NODE_OPTIONS to
'--max-old-space-size=4096', overwriting any existing flags; change the env
assignment so it preserves existing process.env.NODE_OPTIONS by appending (or
prepending) the memory cap to the existing string if present (and avoid
duplicating the flag), e.g., build NODE_OPTIONS from process.env.NODE_OPTIONS
and '--max-old-space-size=4096' before assigning it in the env object used in
the Playwright configuration.
In `@apps/ui/tests/utils/views/profiles.ts`:
- Around line 170-173: The cancel click currently uses a global selector in
cancelProfileDialog which can hit the wrong element; scope the lookup to the
active profile dialog by first locating the dialog/container (e.g., use a dialog
selector that identifies the profile dialog such as an open dialog, heading
text, or class) and then call .locator('button:has-text("Cancel")') or
.getByRole('button', { name: 'Cancel' }) on that dialog locator before clicking;
update the cancelButton definition in cancelProfileDialog so the click is
performed against the scoped locator rather than the global page locator.
- Around line 441-445: The bounds check in dragProfile only guards against
indices >= cards.length but allows negative indices; update dragProfile to also
validate fromIndex and toIndex are >= 0 before proceeding and throw a clear
Error (e.g. including fromIndex, toIndex, total) if either is negative; place
this check alongside the existing upper-bound check so negative indices are
rejected with the same clear runtime message.
- Around line 160-165: The saveProfile helper currently swallows failures from
the two waits which can let tests proceed when the save dialog never closed;
update saveProfile (used with clickElement and waitForElementHidden calls for
'add-profile-dialog' and 'edit-profile-dialog') to await those waits without
silent .catch(), and if either wait rejects rethrow an error (or throw a new
Error with context) so the test fails clearly (include which selector failed to
hide in the thrown message).
---
Duplicate comments:
In `@apps/ui/src/components/views/context-view.tsx`:
- Around line 237-242: handleSelectFile currently calls loadFileContent(file)
unconditionally which discards in-progress edits when hasChanges is true; update
handleSelectFile to check the hasChanges flag and, if true, show a
confirmation/save prompt (e.g., call an existing
promptSaveChanges/confirmDiscard helper or open a modal) and only call
loadFileContent(file) and setIsPreviewMode(...) after the user either saves or
explicitly confirms discarding changes; if the user cancels, abort and keep the
current file and edit state.
In `@apps/ui/src/components/views/memory-view.tsx`:
- Around line 143-148: handleSelectFile currently calls loadFileContent(file)
and setIsPreviewMode(true) unconditionally which allows unsaved edits
(hasChanges) to be discarded; update handleSelectFile to check hasChanges first
and, if true, show a confirmation/save prompt (or call the existing save
routine) and only proceed to call loadFileContent(file) and
setIsPreviewMode(true) when the user confirms discard or after a successful
save; reference the handleSelectFile function and the hasChanges flag, and wire
the modal/confirmation's onConfirm to perform loadFileContent(file) and
setIsPreviewMode(true).
---
Nitpick comments:
In `@apps/ui/tests/projects/board-background-persistence.spec.ts`:
- Around line 187-189: The fixed 2s sleep using page.waitForTimeout(2000) (in
the test that relies on useProjectSettingsLoader) is flaky; replace it with a
deterministic wait such as page.waitForResponse(...) for the settings API
request, or page.waitForSelector(...) / page.waitForFunction(...) that checks a
DOM attribute or global state indicating settings loaded (e.g., a response URL
match for the settings endpoint or a data-testid like "project-settings-loaded"
or window.__projectSettingsLoaded). Update the test to await that signal instead
of page.waitForTimeout and keep the reference to page.waitForTimeout and
useProjectSettingsLoader so reviewers can find the change.
- Around line 73-76: Add a guard around the test fixture copy so missing fixture
errors are clear: before calling fs.copyFileSync with testImagePath and
backgroundPath (the variables named testImagePath and backgroundPath in this
test) check for the file's existence (e.g., fs.existsSync(testImagePath)) and if
it's absent throw or fail the test with a descriptive message like "missing test
fixture ../img/background.jpg" or wrap fs.copyFileSync in a try/catch and
rethrow a new Error that includes that descriptive message plus the original
error; this ensures failures from fs.copyFileSync are explicit about the missing
fixture.
- Around line 319-361: The test currently hardcodes store version numbers
(version: 2 and version: 1) when seeding localStorage in the addInitScript;
replace those literals with the centralized store version constants (e.g.,
STORE_VERSIONS or the existing versions.APP_STORE and versions.SETUP_STORE) or
call the existing setupRealProject helper which already uses the correct
versions; locate the localStorage setup inside the addInitScript and swap the
numeric literals for the appropriate constants (or refactor to use
setupRealProject) so the test stays in sync with STORE_VERSIONS.
In `@apps/ui/tests/utils/project/fixtures.ts`:
- Around line 80-82: The helper that writes fixture files uses
resolveContextFixturePath(filename) and then fs.writeFileSync(filePath, content)
but doesn't ensure parent directories exist, causing ENOENT for nested paths;
before calling fs.writeFileSync in the fixture writer (and the similar writer
around lines 100-101), create the parent directory with something like
fs.mkdirSync(path.dirname(filePath), { recursive: true }) so the directory tree
exists, then proceed to write the file.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/ui/playwright.config.tsapps/ui/src/components/views/context-view.tsxapps/ui/src/components/views/memory-view.tsxapps/ui/src/lib/image-utils.tsapps/ui/tests/profiles/profiles-crud.spec.tsapps/ui/tests/projects/board-background-persistence.spec.tsapps/ui/tests/utils/index.tsapps/ui/tests/utils/project/fixtures.spec.tsapps/ui/tests/utils/project/fixtures.tsapps/ui/tests/utils/views/profiles.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/ui/tests/utils/index.ts
- apps/ui/tests/utils/project/fixtures.spec.ts
Comment on lines
+160
to
+165
| export async function saveProfile(page: Page): Promise<void> { | ||
| await clickElement(page, 'save-profile-button'); | ||
| // Wait for dialog to close | ||
| await waitForElementHidden(page, 'add-profile-dialog').catch(() => {}); | ||
| await waitForElementHidden(page, 'edit-profile-dialog').catch(() => {}); | ||
| } |
Contributor
There was a problem hiding this comment.
saveProfile can mask a failed dialog close.
Lines 163-164 swallow wait failures, so tests may continue even if the save dialog never closes.
💡 Suggested fix
export async function saveProfile(page: Page): Promise<void> {
await clickElement(page, 'save-profile-button');
- // Wait for dialog to close
- await waitForElementHidden(page, 'add-profile-dialog').catch(() => {});
- await waitForElementHidden(page, 'edit-profile-dialog').catch(() => {});
+ // At least one dialog must close; fail if neither does
+ await Promise.any([
+ waitForElementHidden(page, 'add-profile-dialog'),
+ waitForElementHidden(page, 'edit-profile-dialog'),
+ ]);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/tests/utils/views/profiles.ts` around lines 160 - 165, The
saveProfile helper currently swallows failures from the two waits which can let
tests proceed when the save dialog never closed; update saveProfile (used with
clickElement and waitForElementHidden calls for 'add-profile-dialog' and
'edit-profile-dialog') to await those waits without silent .catch(), and if
either wait rejects rethrow an error (or throw a new Error with context) so the
test fails clearly (include which selector failed to hide in the thrown
message).
Comment on lines
+170
to
+173
| export async function cancelProfileDialog(page: Page): Promise<void> { | ||
| // Look for cancel button in dialog footer | ||
| const cancelButton = page.locator('button:has-text("Cancel")'); | ||
| await cancelButton.click(); |
Contributor
There was a problem hiding this comment.
Scope the cancel click to the active profile dialog.
Line 172 uses a global button:has-text("Cancel") selector, which can target the wrong button when multiple dialogs/components are present.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/tests/utils/views/profiles.ts` around lines 170 - 173, The cancel
click currently uses a global selector in cancelProfileDialog which can hit the
wrong element; scope the lookup to the active profile dialog by first locating
the dialog/container (e.g., use a dialog selector that identifies the profile
dialog such as an open dialog, heading text, or class) and then call
.locator('button:has-text("Cancel")') or .getByRole('button', { name: 'Cancel'
}) on that dialog locator before clicking; update the cancelButton definition in
cancelProfileDialog so the click is performed against the scoped locator rather
than the global page locator.
Comment on lines
+441
to
+445
| if (fromIndex >= cards.length || toIndex >= cards.length) { | ||
| throw new Error( | ||
| `Invalid drag indices: fromIndex=${fromIndex}, toIndex=${toIndex}, total=${cards.length}` | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Add negative-index guards in dragProfile.
Current bounds check only validates upper bounds; negative indices can still slip through and fail with less clear runtime errors.
💡 Suggested fix
- if (fromIndex >= cards.length || toIndex >= cards.length) {
+ if (fromIndex < 0 || toIndex < 0 || fromIndex >= cards.length || toIndex >= cards.length) {
throw new Error(
`Invalid drag indices: fromIndex=${fromIndex}, toIndex=${toIndex}, total=${cards.length}`
);
}📝 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.
Suggested change
| if (fromIndex >= cards.length || toIndex >= cards.length) { | |
| throw new Error( | |
| `Invalid drag indices: fromIndex=${fromIndex}, toIndex=${toIndex}, total=${cards.length}` | |
| ); | |
| } | |
| if (fromIndex < 0 || toIndex < 0 || fromIndex >= cards.length || toIndex >= cards.length) { | |
| throw new Error( | |
| `Invalid drag indices: fromIndex=${fromIndex}, toIndex=${toIndex}, total=${cards.length}` | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/tests/utils/views/profiles.ts` around lines 441 - 445, The bounds
check in dragProfile only guards against indices >= cards.length but allows
negative indices; update dragProfile to also validate fromIndex and toIndex are
>= 0 before proceeding and throw a clear Error (e.g. including fromIndex,
toIndex, total) if either is negative; place this check alongside the existing
upper-bound check so negative indices are rejected with the same clear runtime
message.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
test/running-task-display-test-805-6c4ockc/test-project-1772088506096/test-feature.test.js (1)
18-18: Prefer public API over internal field checks.Line 18, Line 34, and Line 66 read
feature.statusdirectly. SincegetStatus()exists, using it here reduces coupling to internal representation.♻️ Suggested minimal refactor
- if (feature.name === 'Test Feature' && feature.status === 'running') { + if (feature.name === 'Test Feature' && feature.getStatus() === 'running') { - if (result.success === true && feature.status === 'completed') { + if (result.success === true && feature.getStatus() === 'completed') { - if (info.name === 'My Test Feature' && info.status === 'running' && info.createdAt) { + if (info.name === 'My Test Feature' && feature.getStatus() === 'running' && info.createdAt) {Also applies to: 34-34, 66-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/running-task-display-test-805-6c4ockc/test-project-1772088506096/test-feature.test.js` at line 18, Replace direct reads of the internal field feature.status with the public accessor feature.getStatus() in the test assertions and conditionals (e.g., change checks like feature.status === 'running' to feature.getStatus() === 'running'); update all occurrences that reference feature.status (used in the test cases where feature is defined) to call getStatus() instead so the tests rely on the public API rather than internal representation.test/running-task-display-test-805-6c4ockc/test-project-1772088506096/test-feature.js (1)
20-20: Avoid hardwired logging inexecute().Line 20 logs on every execution, which adds noise in automated runs. Consider injecting a logger or making logging optional.
🔧 Lightweight option
- execute() { - console.log(`Executing ${this.name}...`); + execute({ logger } = {}) { + logger?.info?.(`Executing ${this.name}...`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/running-task-display-test-805-6c4ockc/test-project-1772088506096/test-feature.js` at line 20, The execute() method contains a hardwired console.log(`Executing ${this.name}...`) which spams automated runs; change it to use an injected logger or an optional flag instead: add a constructor/property (e.g., this.logger or this.options.logger and/or this.options.silent) and replace the console.log with a conditional call like this.logger?.info(`Executing ${this.name}...`) or guard by if (!this.options.silent) then call logger/noop; ensure execute() and any instantiation sites pass or default the logger/flag so existing behavior is preserved when desired.apps/ui/tests/utils/navigation/views.ts (1)
15-20: Pre-navigation synchronization pattern applied inconsistently.The new synchronization pattern (waitForLoadState + small delay before goto) is added to
navigateToBoard,navigateToContext,navigateToSpec, andnavigateToAgent, but not tonavigateToSettings,navigateToSetup, ornavigateToWelcome. If this pattern prevents race conditions on mobile viewports, consider applying it consistently across all navigation helpers for uniform behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/utils/navigation/views.ts` around lines 15 - 20, The pre-navigation synchronization (await page.waitForLoadState('domcontentloaded').catch(() => {}); await page.waitForTimeout(100);) is applied in navigateToBoard, navigateToContext, navigateToSpec, and navigateToAgent but missing from navigateToSettings, navigateToSetup, and navigateToWelcome; add the same two-line wait pattern immediately before the page.goto call in each of those three functions (navigateToSettings, navigateToSetup, navigateToWelcome) so all navigation helpers use the same pre-goto synchronization to avoid mobile race conditions.apps/ui/tests/projects/board-background-persistence.spec.ts (2)
26-27: Module-level temp directory may cause parallel test issues.
TEST_TEMP_DIRis created at module load time with a timestamp. If Playwright runs multiple workers that load this module, they'll each get unique directories (due toDate.now()increateTempDirPath), but thebeforeAll/afterAllhooks are per-describe block. This should work correctly, but consider documenting that this test file should run in isolation if issues arise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/projects/board-background-persistence.spec.ts` around lines 26 - 27, TEST_TEMP_DIR is created at module load time via createTempDirPath which can cause cross-worker timing/initialization issues in parallel test runs; move the temp-dir creation into the test lifecycle by declaring a let TEST_TEMP_DIR variable at module scope and initializing it inside the beforeAll hook (and cleaning it up in afterAll) so each worker/describe creates and removes its own directory; reference createTempDirPath, TEST_TEMP_DIR, beforeAll and afterAll to locate and update the code.
192-194: Consider replacing hardcoded timeouts with explicit waits.The 2000ms timeouts (lines 194, 251, 410) for settings loading are flakiness risks. Since you're already tracking
settingsApiCalls, you could wait for the specific API call to complete instead:// Example: Wait for settings API call instead of fixed timeout await page.waitForRequest((req) => req.url().includes('/api/settings/project') && req.postData()?.includes(projectAPath) );This is a suggested improvement for test reliability rather than a blocking issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/projects/board-background-persistence.spec.ts` around lines 192 - 194, Replace the brittle fixed delays that call page.waitForTimeout(2000) with an explicit wait for the settings API request/response used by the useProjectSettingsLoader hook (e.g., use page.waitForRequest or page.waitForResponse filtering on the settings endpoint like '/api/settings/project' and the project path), or wait on the existing settingsApiCalls tracker to reach the expected count; update all occurrences of page.waitForTimeout in board-background-persistence.spec.ts to await the specific request/response or tracker instead of the hardcoded 2000ms so the background settings loading is observed deterministically.apps/ui/src/components/views/context-view.tsx (1)
974-987: Consider adding a mobile dropdown for file actions in the toolbar.Currently on mobile, deleting the selected file requires navigating back to the file list first. While functional, adding a compact dropdown menu (e.g., with MoreVertical icon) in the mobile toolbar could streamline the workflow. This is a nice-to-have for future iteration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/context-view.tsx` around lines 974 - 987, Add a compact mobile action dropdown to the toolbar so mobile users can delete the selected file without navigating back: when isMobile is true, replace or augment the Desktop-only Button block (the Button with data-testid="delete-context-file", onClick calling setIsDeleteDialogOpen(true) and icon Trash2) with a small dropdown trigger (e.g., MoreVertical icon) that opens a menu containing the Delete action which calls setIsDeleteDialogOpen(true); ensure the dropdown is only rendered for mobile and keeps the existing desktop Button unchanged, and add appropriate aria-label/testid for the mobile menu item for testability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ui/src/components/views/context-view.tsx`:
- Around line 800-810: The desktop width class (w-64) in
FILE_LIST_DESKTOP_CLASSES conflicts with FILE_LIST_EXPANDED_CLASSES (flex-1)
when no file is selected; update the conditional class application so that the
fixed width is only applied when a file is selected—e.g., remove or split w-64
out of FILE_LIST_DESKTOP_CLASSES and include that narrow-desktop class only when
selectedFile is truthy, while keeping FILE_LIST_EXPANDED_CLASSES (flex-1) when
!selectedFile; adjust the class expression around
FILE_LIST_DESKTOP_CLASSES/FILE_LIST_EXPANDED_CLASSES in the component rendering
the div (the code referencing selectedFile, isMobile, FILE_LIST_BASE_CLASSES,
FILE_LIST_DESKTOP_CLASSES, FILE_LIST_EXPANDED_CLASSES,
FILE_LIST_MOBILE_NO_SELECTION_CLASSES, and FILE_LIST_MOBILE_SELECTION_CLASSES).
---
Nitpick comments:
In `@apps/ui/src/components/views/context-view.tsx`:
- Around line 974-987: Add a compact mobile action dropdown to the toolbar so
mobile users can delete the selected file without navigating back: when isMobile
is true, replace or augment the Desktop-only Button block (the Button with
data-testid="delete-context-file", onClick calling setIsDeleteDialogOpen(true)
and icon Trash2) with a small dropdown trigger (e.g., MoreVertical icon) that
opens a menu containing the Delete action which calls
setIsDeleteDialogOpen(true); ensure the dropdown is only rendered for mobile and
keeps the existing desktop Button unchanged, and add appropriate
aria-label/testid for the mobile menu item for testability.
In `@apps/ui/tests/projects/board-background-persistence.spec.ts`:
- Around line 26-27: TEST_TEMP_DIR is created at module load time via
createTempDirPath which can cause cross-worker timing/initialization issues in
parallel test runs; move the temp-dir creation into the test lifecycle by
declaring a let TEST_TEMP_DIR variable at module scope and initializing it
inside the beforeAll hook (and cleaning it up in afterAll) so each
worker/describe creates and removes its own directory; reference
createTempDirPath, TEST_TEMP_DIR, beforeAll and afterAll to locate and update
the code.
- Around line 192-194: Replace the brittle fixed delays that call
page.waitForTimeout(2000) with an explicit wait for the settings API
request/response used by the useProjectSettingsLoader hook (e.g., use
page.waitForRequest or page.waitForResponse filtering on the settings endpoint
like '/api/settings/project' and the project path), or wait on the existing
settingsApiCalls tracker to reach the expected count; update all occurrences of
page.waitForTimeout in board-background-persistence.spec.ts to await the
specific request/response or tracker instead of the hardcoded 2000ms so the
background settings loading is observed deterministically.
In `@apps/ui/tests/utils/navigation/views.ts`:
- Around line 15-20: The pre-navigation synchronization (await
page.waitForLoadState('domcontentloaded').catch(() => {}); await
page.waitForTimeout(100);) is applied in navigateToBoard, navigateToContext,
navigateToSpec, and navigateToAgent but missing from navigateToSettings,
navigateToSetup, and navigateToWelcome; add the same two-line wait pattern
immediately before the page.goto call in each of those three functions
(navigateToSettings, navigateToSetup, navigateToWelcome) so all navigation
helpers use the same pre-goto synchronization to avoid mobile race conditions.
In
`@test/running-task-display-test-805-6c4ockc/test-project-1772088506096/test-feature.js`:
- Line 20: The execute() method contains a hardwired console.log(`Executing
${this.name}...`) which spams automated runs; change it to use an injected
logger or an optional flag instead: add a constructor/property (e.g.,
this.logger or this.options.logger and/or this.options.silent) and replace the
console.log with a conditional call like this.logger?.info(`Executing
${this.name}...`) or guard by if (!this.options.silent) then call logger/noop;
ensure execute() and any instantiation sites pass or default the logger/flag so
existing behavior is preserved when desired.
In
`@test/running-task-display-test-805-6c4ockc/test-project-1772088506096/test-feature.test.js`:
- Line 18: Replace direct reads of the internal field feature.status with the
public accessor feature.getStatus() in the test assertions and conditionals
(e.g., change checks like feature.status === 'running' to feature.getStatus()
=== 'running'); update all occurrences that reference feature.status (used in
the test cases where feature is defined) to call getStatus() instead so the
tests rely on the public API rather than internal representation.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/ui/src/components/views/context-view.tsxapps/ui/tests/projects/board-background-persistence.spec.tsapps/ui/tests/utils/navigation/views.tsapps/ui/tests/utils/project/fixtures.tsapps/ui/tests/utils/views/memory.tstest/opus-thinking-level-none-83449-2bgz7j1/test-project-1772086066067/package.jsontest/running-task-display-test-805-6c4ockc/test-project-1772088506096/README.mdtest/running-task-display-test-805-6c4ockc/test-project-1772088506096/test-feature.jstest/running-task-display-test-805-6c4ockc/test-project-1772088506096/test-feature.test.js
✅ Files skipped from review due to trivial changes (2)
- test/opus-thinking-level-none-83449-2bgz7j1/test-project-1772086066067/package.json
- test/running-task-display-test-805-6c4ockc/test-project-1772088506096/README.md
Comment on lines
+800
to
+810
| {/* Mobile: Full width, hidden when file is selected (full-screen editor) */} | ||
| {/* Desktop: Fixed width w-64, expands to fill space when no file selected */} | ||
| <div | ||
| className={cn( | ||
| FILE_LIST_BASE_CLASSES, | ||
| FILE_LIST_DESKTOP_CLASSES, | ||
| !selectedFile && FILE_LIST_EXPANDED_CLASSES, | ||
| isMobile && !selectedFile && FILE_LIST_MOBILE_NO_SELECTION_CLASSES, | ||
| isMobile && selectedFile && FILE_LIST_MOBILE_SELECTION_CLASSES | ||
| )} | ||
| > |
Contributor
There was a problem hiding this comment.
Potential CSS conflict between w-64 and flex-1 on desktop when no file is selected.
When no file is selected on desktop, both w-64 (width: 16rem) and flex-1 (flex: 1 1 0%) are applied. These can produce inconsistent behavior: flex-basis: 0% may override or conflict with the explicit width, depending on browser implementation.
Per the comment on line 801, the intent is for the file list to expand when no file is selected. Consider applying w-64 only when a file is selected:
🔧 Proposed fix
<div
className={cn(
FILE_LIST_BASE_CLASSES,
- FILE_LIST_DESKTOP_CLASSES,
- !selectedFile && FILE_LIST_EXPANDED_CLASSES,
+ !isMobile && selectedFile && FILE_LIST_DESKTOP_CLASSES,
+ !isMobile && !selectedFile && FILE_LIST_EXPANDED_CLASSES,
isMobile && !selectedFile && FILE_LIST_MOBILE_NO_SELECTION_CLASSES,
isMobile && selectedFile && FILE_LIST_MOBILE_SELECTION_CLASSES
)}
>📝 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.
Suggested change
| {/* Mobile: Full width, hidden when file is selected (full-screen editor) */} | |
| {/* Desktop: Fixed width w-64, expands to fill space when no file selected */} | |
| <div | |
| className={cn( | |
| FILE_LIST_BASE_CLASSES, | |
| FILE_LIST_DESKTOP_CLASSES, | |
| !selectedFile && FILE_LIST_EXPANDED_CLASSES, | |
| isMobile && !selectedFile && FILE_LIST_MOBILE_NO_SELECTION_CLASSES, | |
| isMobile && selectedFile && FILE_LIST_MOBILE_SELECTION_CLASSES | |
| )} | |
| > | |
| {/* Mobile: Full width, hidden when file is selected (full-screen editor) */} | |
| {/* Desktop: Fixed width w-64, expands to fill space when no file selected */} | |
| <div | |
| className={cn( | |
| FILE_LIST_BASE_CLASSES, | |
| !isMobile && selectedFile && FILE_LIST_DESKTOP_CLASSES, | |
| !isMobile && !selectedFile && FILE_LIST_EXPANDED_CLASSES, | |
| isMobile && !selectedFile && FILE_LIST_MOBILE_NO_SELECTION_CLASSES, | |
| isMobile && selectedFile && FILE_LIST_MOBILE_SELECTION_CLASSES | |
| )} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/components/views/context-view.tsx` around lines 800 - 810, The
desktop width class (w-64) in FILE_LIST_DESKTOP_CLASSES conflicts with
FILE_LIST_EXPANDED_CLASSES (flex-1) when no file is selected; update the
conditional class application so that the fixed width is only applied when a
file is selected—e.g., remove or split w-64 out of FILE_LIST_DESKTOP_CLASSES and
include that narrow-desktop class only when selectedFile is truthy, while
keeping FILE_LIST_EXPANDED_CLASSES (flex-1) when !selectedFile; adjust the class
expression around FILE_LIST_DESKTOP_CLASSES/FILE_LIST_EXPANDED_CLASSES in the
component rendering the div (the code referencing selectedFile, isMobile,
FILE_LIST_BASE_CLASSES, FILE_LIST_DESKTOP_CLASSES, FILE_LIST_EXPANDED_CLASSES,
FILE_LIST_MOBILE_NO_SELECTION_CLASSES, and FILE_LIST_MOBILE_SELECTION_CLASSES).
gsxdsm
added a commit
that referenced
this pull request
Feb 26, 2026
gsxdsm
added a commit
that referenced
this pull request
Feb 26, 2026
* Changes from fix/memory-and-context-mobile-friendly * fix: Improve file extension detection and add path traversal protection * refactor: Extract file extension utilities and add path traversal guards Code review improvements: - Extract isMarkdownFilename and isImageFilename to shared image-utils.ts - Remove duplicated code from context-view.tsx and memory-view.tsx - Add path traversal guard for context fixture utilities (matching memory) - Add 7 new tests for context fixture path traversal protection - Total 61 tests pass Addresses code review feedback from PR #813 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: Add e2e tests for profiles crud and board background persistence * Update apps/ui/playwright.config.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: Add robust test navigation handling and file filtering * fix: Format NODE_OPTIONS configuration on single line * test: Update profiles and board background persistence tests * test: Replace iPhone 13 Pro with Pixel 5 for mobile test consistency * Update apps/ui/src/components/views/context-view.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: Remove test project directory * feat: Filter context files by type and improve mobile menu visibility --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
gsxdsm
added a commit
to gsxdsm/automaker
that referenced
this pull request
Feb 26, 2026
* Changes from fix/memory-and-context-mobile-friendly * fix: Improve file extension detection and add path traversal protection * refactor: Extract file extension utilities and add path traversal guards Code review improvements: - Extract isMarkdownFilename and isImageFilename to shared image-utils.ts - Remove duplicated code from context-view.tsx and memory-view.tsx - Add path traversal guard for context fixture utilities (matching memory) - Add 7 new tests for context fixture path traversal protection - Total 61 tests pass Addresses code review feedback from PR AutoMaker-Org#813 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: Add e2e tests for profiles crud and board background persistence * Update apps/ui/playwright.config.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: Add robust test navigation handling and file filtering * fix: Format NODE_OPTIONS configuration on single line * test: Update profiles and board background persistence tests * test: Replace iPhone 13 Pro with Pixel 5 for mobile test consistency * Update apps/ui/src/components/views/context-view.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: Remove test project directory * feat: Filter context files by type and improve mobile menu visibility --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
gsxdsm
added a commit
that referenced
this pull request
Feb 28, 2026
* fix(copilot): correct tool.execution_complete event handling The CopilotProvider was using incorrect event type and data structure for tool execution completion events from the @github/copilot-sdk, causing tool call outputs to be empty. Changes: - Update event type from 'tool.execution_end' to 'tool.execution_complete' - Fix data structure to use nested result.content instead of flat result - Fix error structure to use error.message instead of flat error - Add success field to match SDK event structure - Add tests for empty and missing result handling This aligns with the official @github/copilot-sdk v0.1.16 types defined in session-events.d.ts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(copilot): add edge case test for error with code field Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor(copilot): improve error handling and code quality Code review improvements: - Extract magic string '[ERROR]' to TOOL_ERROR_PREFIX constant - Add null-safe error handling with direct error variable assignment - Include error codes in error messages for better debugging - Add JSDoc documentation for tool.execution_complete handler - Update tests to verify error codes are displayed - Add missing tool_use_id assertion in error test These changes improve: - Code maintainability (no magic strings) - Debugging experience (error codes now visible) - Type safety (explicit null checks) - Test coverage (verify error code formatting) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Changes from fix/bug-fixes-1-0 * test(copilot): add edge case test for error with code field Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Changes from fix/bug-fixes-1-0 * fix: Handle detached HEAD state in worktree discovery and recovery * fix: Remove unused isDevServerStarting prop and md: breakpoint classes * fix: Add missing dependency and sanitize persisted cache data * feat: Ensure NODE_ENV is set to test in vitest configs * feat: Configure Playwright to run only E2E tests * fix: Improve PR tracking and dev server lifecycle management * feat: Add settings-based defaults for planning mode, model config, and custom providers. Fixes #816 * feat: Add worktree and branch selector to graph view * fix: Add timeout and error handling for worktree HEAD ref resolution * fix: use absolute icon path and place icon outside asar on Linux The hicolor icon theme index only lists sizes up to 512x512, so an icon installed only at 1024x1024 is invisible to GNOME/KDE's theme resolver, causing both the app launcher and taskbar to show a generic icon. Additionally, BrowserWindow.icon cannot be read by the window manager when the file is inside app.asar. - extraResources: copy logo_larger.png to resources/ (outside asar) so it lands at /opt/Automaker/resources/logo_larger.png on install - linux.desktop.Icon: set to the absolute resources path, bypassing the hicolor theme lookup and its size constraints entirely - icon-manager.ts: on Linux production use process.resourcesPath so BrowserWindow receives a real filesystem path the WM can read directly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: use linux.desktop.entry for custom desktop Icon field electron-builder v26 rejects arbitrary keys in linux.desktop — the correct schema wraps custom .desktop overrides inside desktop.entry. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: set desktop name on Linux so taskbar uses the correct app icon Without app.setDesktopName(), the window manager cannot associate the running Electron process with automaker.desktop. GNOME/KDE fall back to _NET_WM_ICON which defaults to Electron's own bundled icon. Calling app.setDesktopName('automaker.desktop') before any window is created sets the _GTK_APPLICATION_ID hint and XDG app_id so the WM picks up the desktop entry's Icon for the taskbar. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix: memory and context views mobile friendly (#818) * Changes from fix/memory-and-context-mobile-friendly * fix: Improve file extension detection and add path traversal protection * refactor: Extract file extension utilities and add path traversal guards Code review improvements: - Extract isMarkdownFilename and isImageFilename to shared image-utils.ts - Remove duplicated code from context-view.tsx and memory-view.tsx - Add path traversal guard for context fixture utilities (matching memory) - Add 7 new tests for context fixture path traversal protection - Total 61 tests pass Addresses code review feedback from PR #813 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: Add e2e tests for profiles crud and board background persistence * Update apps/ui/playwright.config.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: Add robust test navigation handling and file filtering * fix: Format NODE_OPTIONS configuration on single line * test: Update profiles and board background persistence tests * test: Replace iPhone 13 Pro with Pixel 5 for mobile test consistency * Update apps/ui/src/components/views/context-view.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: Remove test project directory * feat: Filter context files by type and improve mobile menu visibility --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: Improve test reliability and localhost handling * chore: Use explicit TEST_USE_EXTERNAL_BACKEND env var for server cleanup * feat: Add E2E/CI mock mode for provider factory and auth verification * feat: Add remoteBranch parameter to pull and rebase operations * chore: Enhance E2E testing setup with worker isolation and auth state management - Updated .gitignore to include worker-specific test fixtures. - Modified e2e-tests.yml to implement test sharding for improved CI performance. - Refactored global setup to authenticate once and save session state for reuse across tests. - Introduced worker-isolated fixture paths to prevent conflicts during parallel test execution. - Improved test navigation and loading handling for better reliability. - Updated various test files to utilize new auth state management and fixture paths. * fix: Update Playwright configuration and improve test reliability - Increased the number of workers in Playwright configuration for better parallelism in CI environments. - Enhanced the board background persistence test to ensure dropdown stability by waiting for the list to populate before interaction, improving test reliability. * chore: Simplify E2E test configuration and enhance mock implementations - Updated e2e-tests.yml to run tests in a single shard for streamlined CI execution. - Enhanced unit tests for worktree list handling by introducing a mock for execGitCommand, improving test reliability and coverage. - Refactored setup functions to better manage command mocks for git operations in tests. - Improved error handling in mkdirSafe function to account for undefined stats in certain environments. * refactor: Improve test configurations and enhance error handling - Updated Playwright configuration to clear VITE_SERVER_URL, ensuring the frontend uses the Vite proxy and preventing cookie domain mismatches. - Enhanced MergeRebaseDialog logic to normalize selectedBranch for better handling of various ref formats. - Improved global setup with a more robust backend health check, throwing an error if the backend is not healthy after retries. - Refactored project creation tests to handle file existence checks more reliably. - Added error handling for missing E2E source fixtures to guide setup process. - Enhanced memory navigation to handle sandbox dialog visibility more effectively. * refactor: Enhance Git command execution and improve test configurations - Updated Git command execution to merge environment paths correctly, ensuring proper command execution context. - Refactored the Git initialization process to handle errors more gracefully and ensure user configuration is set before creating the initial commit. - Improved test configurations by updating Playwright test identifiers for better clarity and consistency across different project states. - Enhanced cleanup functions in tests to handle directory removal more robustly, preventing errors during test execution. * fix: Resolve React hooks errors from duplicate instances in dependency tree * style: Format alias configuration for improved readability --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: DhanushSantosh <dhanushsantoshs05@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
gsxdsm
added a commit
that referenced
this pull request
Mar 3, 2026
* Changes from fix/memory-and-context-mobile-friendly * fix: Improve file extension detection and add path traversal protection * refactor: Extract file extension utilities and add path traversal guards Code review improvements: - Extract isMarkdownFilename and isImageFilename to shared image-utils.ts - Remove duplicated code from context-view.tsx and memory-view.tsx - Add path traversal guard for context fixture utilities (matching memory) - Add 7 new tests for context fixture path traversal protection - Total 61 tests pass Addresses code review feedback from PR #813 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: Add e2e tests for profiles crud and board background persistence * Update apps/ui/playwright.config.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: Add robust test navigation handling and file filtering * fix: Format NODE_OPTIONS configuration on single line * test: Update profiles and board background persistence tests * test: Replace iPhone 13 Pro with Pixel 5 for mobile test consistency * Update apps/ui/src/components/views/context-view.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: Remove test project directory * feat: Filter context files by type and improve mobile menu visibility --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
gsxdsm
added a commit
that referenced
this pull request
Mar 3, 2026
* fix(copilot): correct tool.execution_complete event handling The CopilotProvider was using incorrect event type and data structure for tool execution completion events from the @github/copilot-sdk, causing tool call outputs to be empty. Changes: - Update event type from 'tool.execution_end' to 'tool.execution_complete' - Fix data structure to use nested result.content instead of flat result - Fix error structure to use error.message instead of flat error - Add success field to match SDK event structure - Add tests for empty and missing result handling This aligns with the official @github/copilot-sdk v0.1.16 types defined in session-events.d.ts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(copilot): add edge case test for error with code field Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor(copilot): improve error handling and code quality Code review improvements: - Extract magic string '[ERROR]' to TOOL_ERROR_PREFIX constant - Add null-safe error handling with direct error variable assignment - Include error codes in error messages for better debugging - Add JSDoc documentation for tool.execution_complete handler - Update tests to verify error codes are displayed - Add missing tool_use_id assertion in error test These changes improve: - Code maintainability (no magic strings) - Debugging experience (error codes now visible) - Type safety (explicit null checks) - Test coverage (verify error code formatting) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Changes from fix/bug-fixes-1-0 * test(copilot): add edge case test for error with code field Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Changes from fix/bug-fixes-1-0 * fix: Handle detached HEAD state in worktree discovery and recovery * fix: Remove unused isDevServerStarting prop and md: breakpoint classes * fix: Add missing dependency and sanitize persisted cache data * feat: Ensure NODE_ENV is set to test in vitest configs * feat: Configure Playwright to run only E2E tests * fix: Improve PR tracking and dev server lifecycle management * feat: Add settings-based defaults for planning mode, model config, and custom providers. Fixes #816 * feat: Add worktree and branch selector to graph view * fix: Add timeout and error handling for worktree HEAD ref resolution * fix: use absolute icon path and place icon outside asar on Linux The hicolor icon theme index only lists sizes up to 512x512, so an icon installed only at 1024x1024 is invisible to GNOME/KDE's theme resolver, causing both the app launcher and taskbar to show a generic icon. Additionally, BrowserWindow.icon cannot be read by the window manager when the file is inside app.asar. - extraResources: copy logo_larger.png to resources/ (outside asar) so it lands at /opt/Automaker/resources/logo_larger.png on install - linux.desktop.Icon: set to the absolute resources path, bypassing the hicolor theme lookup and its size constraints entirely - icon-manager.ts: on Linux production use process.resourcesPath so BrowserWindow receives a real filesystem path the WM can read directly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: use linux.desktop.entry for custom desktop Icon field electron-builder v26 rejects arbitrary keys in linux.desktop — the correct schema wraps custom .desktop overrides inside desktop.entry. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: set desktop name on Linux so taskbar uses the correct app icon Without app.setDesktopName(), the window manager cannot associate the running Electron process with automaker.desktop. GNOME/KDE fall back to _NET_WM_ICON which defaults to Electron's own bundled icon. Calling app.setDesktopName('automaker.desktop') before any window is created sets the _GTK_APPLICATION_ID hint and XDG app_id so the WM picks up the desktop entry's Icon for the taskbar. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix: memory and context views mobile friendly (#818) * Changes from fix/memory-and-context-mobile-friendly * fix: Improve file extension detection and add path traversal protection * refactor: Extract file extension utilities and add path traversal guards Code review improvements: - Extract isMarkdownFilename and isImageFilename to shared image-utils.ts - Remove duplicated code from context-view.tsx and memory-view.tsx - Add path traversal guard for context fixture utilities (matching memory) - Add 7 new tests for context fixture path traversal protection - Total 61 tests pass Addresses code review feedback from PR #813 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: Add e2e tests for profiles crud and board background persistence * Update apps/ui/playwright.config.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: Add robust test navigation handling and file filtering * fix: Format NODE_OPTIONS configuration on single line * test: Update profiles and board background persistence tests * test: Replace iPhone 13 Pro with Pixel 5 for mobile test consistency * Update apps/ui/src/components/views/context-view.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: Remove test project directory * feat: Filter context files by type and improve mobile menu visibility --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: Improve test reliability and localhost handling * chore: Use explicit TEST_USE_EXTERNAL_BACKEND env var for server cleanup * feat: Add E2E/CI mock mode for provider factory and auth verification * feat: Add remoteBranch parameter to pull and rebase operations * chore: Enhance E2E testing setup with worker isolation and auth state management - Updated .gitignore to include worker-specific test fixtures. - Modified e2e-tests.yml to implement test sharding for improved CI performance. - Refactored global setup to authenticate once and save session state for reuse across tests. - Introduced worker-isolated fixture paths to prevent conflicts during parallel test execution. - Improved test navigation and loading handling for better reliability. - Updated various test files to utilize new auth state management and fixture paths. * fix: Update Playwright configuration and improve test reliability - Increased the number of workers in Playwright configuration for better parallelism in CI environments. - Enhanced the board background persistence test to ensure dropdown stability by waiting for the list to populate before interaction, improving test reliability. * chore: Simplify E2E test configuration and enhance mock implementations - Updated e2e-tests.yml to run tests in a single shard for streamlined CI execution. - Enhanced unit tests for worktree list handling by introducing a mock for execGitCommand, improving test reliability and coverage. - Refactored setup functions to better manage command mocks for git operations in tests. - Improved error handling in mkdirSafe function to account for undefined stats in certain environments. * refactor: Improve test configurations and enhance error handling - Updated Playwright configuration to clear VITE_SERVER_URL, ensuring the frontend uses the Vite proxy and preventing cookie domain mismatches. - Enhanced MergeRebaseDialog logic to normalize selectedBranch for better handling of various ref formats. - Improved global setup with a more robust backend health check, throwing an error if the backend is not healthy after retries. - Refactored project creation tests to handle file existence checks more reliably. - Added error handling for missing E2E source fixtures to guide setup process. - Enhanced memory navigation to handle sandbox dialog visibility more effectively. * refactor: Enhance Git command execution and improve test configurations - Updated Git command execution to merge environment paths correctly, ensuring proper command execution context. - Refactored the Git initialization process to handle errors more gracefully and ensure user configuration is set before creating the initial commit. - Improved test configurations by updating Playwright test identifiers for better clarity and consistency across different project states. - Enhanced cleanup functions in tests to handle directory removal more robustly, preventing errors during test execution. * fix: Resolve React hooks errors from duplicate instances in dependency tree * style: Format alias configuration for improved readability --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: DhanushSantosh <dhanushsantoshs05@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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.
Summary
Changes
useIsMobile()hook to both MemoryView and ContextView components for responsive behaviorgap-1) for compact layoutisMarkdownFile,isImageFile) to module-level constants for code reuse and consistencydata-testidattribute to HeaderActionsPanelTrigger for test coverageSummary by CodeRabbit
New Features
Tests
Bug Fixes
Chores