Conversation
… scaling logic for electron and web
Summary of ChangesHello @trueheads, 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 delivers a more fluid and robust user experience for the Kanban board, particularly within the Electron desktop environment. It addresses layout challenges by implementing dynamic column scaling and intelligent window management, ensuring the application adapts seamlessly to various window sizes and user interactions. The changes also lay the groundwork for future server-side functionalities with the introduction of a new server bundle. 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. 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
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a server-bundle package.json; persists, validates, and restores Electron window bounds; exposes an IPC to update minimum width and calls it from the sidebar auto-collapse hook; refactors responsive Kanban sizing (ResizeObserver, containerStyle, new metrics); expands KanbanBoard props; updates context/file tests and various UI styling/formatting. Changes
Sequence Diagram(s)sequenceDiagram
participant Sidebar as Sidebar (renderer)
participant Hook as use-sidebar-auto-collapse (renderer)
participant Preload as Preload (renderer bridge)
participant Main as Electron Main
participant Window as BrowserWindow
Sidebar->>Hook: sidebarOpen changes
Hook->>Preload: electronAPI.updateMinWidth(sidebarOpen)
Preload->>Main: invoke 'window:updateMinWidth' with sidebarOpen
Main->>Main: compute newMinWidth (kanban constants)
Main->>Window: setMinimumSize(newMinWidth, minHeight)
alt current width < newMinWidth
Main->>Window: resize to newMinWidth
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to the Kanban board's responsiveness, particularly for the Electron desktop application. It adds robust logic for persisting window size and position, and dynamically adjusts the window's minimum width based on the sidebar's state to ensure a clean layout. The column sizing logic has been enhanced for better performance and accuracy. My review includes a high-severity comment about a potential regression in column width handling that also causes a test failure, and a medium-severity comment to correct some misleading code comments for better maintainability. Overall, this is a high-quality contribution that greatly improves the application's user experience.
| const BOARD_CONTENT_MIN = | ||
| COLUMN_MIN_WIDTH * COLUMN_COUNT + GAP_SIZE * (COLUMN_COUNT - 1) + BOARD_PADDING; | ||
| const MIN_WIDTH_EXPANDED = BOARD_CONTENT_MIN + SIDEBAR_EXPANDED; // 1500px | ||
| const MIN_WIDTH_COLLAPSED = BOARD_CONTENT_MIN + SIDEBAR_COLLAPSED; // 1276px |
There was a problem hiding this comment.
The comments for the calculated minimum widths are slightly incorrect, which could be misleading for future maintenance.
BOARD_CONTENT_MINis280 * 4 + 20 * 3 + 40 = 1220.MIN_WIDTH_EXPANDEDis1220 + 288 = 1508px(comment on line 49 says 1500px).MIN_WIDTH_COLLAPSEDis1220 + 64 = 1284px(comment on line 50 says 1276px).
Similarly, the comment on line 406 for minWidth should be updated from 1500px to 1508px.
Please update these comments to reflect the correct values.
| const BOARD_CONTENT_MIN = | |
| COLUMN_MIN_WIDTH * COLUMN_COUNT + GAP_SIZE * (COLUMN_COUNT - 1) + BOARD_PADDING; | |
| const MIN_WIDTH_EXPANDED = BOARD_CONTENT_MIN + SIDEBAR_EXPANDED; // 1500px | |
| const MIN_WIDTH_COLLAPSED = BOARD_CONTENT_MIN + SIDEBAR_COLLAPSED; // 1276px | |
| const BOARD_CONTENT_MIN = | |
| COLUMN_MIN_WIDTH * COLUMN_COUNT + GAP_SIZE * (COLUMN_COUNT - 1) + BOARD_PADDING; | |
| const MIN_WIDTH_EXPANDED = BOARD_CONTENT_MIN + SIDEBAR_EXPANDED; // 1508px | |
| const MIN_WIDTH_COLLAPSED = BOARD_CONTENT_MIN + SIDEBAR_COLLAPSED; // 1284px |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/ui/src/components/layout/sidebar.tsx (1)
378-389: LGTM! Consider adding error handling for robustness.The implementation correctly checks for the Electron API before calling
updateMinWidth. However, sinceupdateMinWidthreturns a Promise, unhandled rejections could occur if the IPC call fails.🔎 Optional: Add error handling
useEffect(() => { const electronAPI = ( window as unknown as { electronAPI?: { updateMinWidth?: (expanded: boolean) => Promise<void> }; } ).electronAPI; if (electronAPI?.updateMinWidth) { - electronAPI.updateMinWidth(sidebarOpen); + electronAPI.updateMinWidth(sidebarOpen).catch((err) => { + console.warn('[Sidebar] Failed to update min width:', err); + }); } }, [sidebarOpen]);apps/ui/src/main.ts (1)
55-62: Consider importing WindowBounds from shared types.This interface duplicates the
WindowBoundstype defined inlibs/types/src/settings.ts. While the duplication works, importing from the shared types package would ensure consistency.🔎 Optional: Import from shared types
+import type { WindowBounds } from '@automaker/types'; + -// Window bounds interface (matches @automaker/types WindowBounds) -interface WindowBounds { - x: number; - y: number; - width: number; - height: number; - isMaximized: boolean; -}apps/ui/src/hooks/use-responsive-kanban.ts (2)
57-93: DOM query in calculation function may return stale or null references.The
calculateColumnWidthfunction queries the DOM viadocument.querySelectoron every call. If the element isn't mounted yet (e.g., during initial render or SSR),boardContainercould be null, falling back towindow.innerWidth. This is handled, but the fallback may cause a brief incorrect width before the container mounts.Also, using
data-testidfor functional DOM queries couples test infrastructure to runtime behavior. Consider using a ref passed from the component or a dedicateddata-*attribute for this purpose.
131-144: ResizeObserver may not find the container on initial mount.The DOM query runs once when the effect mounts. If
[data-testid="board-view"]or its parent isn't in the DOM yet,containerwill be null and the observer won't be set up. The effect won't re-run unlesscalculateColumnWidthidentity changes.Consider adding the container element as a dependency or using a ref callback pattern to ensure the observer attaches when the element mounts.
🔎 Potential approach using a callback ref
// In the hook, expose a ref callback for the consumer to attach const containerRef = useRef<HTMLElement | null>(null); const setContainerRef = useCallback((node: HTMLElement | null) => { containerRef.current = node; if (node) { // Trigger recalculation when container is available setColumnWidth(calculateColumnWidth(node.clientWidth)); } }, [calculateColumnWidth]); // In the ResizeObserver effect, use containerRef.current instead of DOM queryThis would require the consumer (KanbanBoard) to attach the ref, but provides more reliable container tracking.
apps/ui/src/components/views/board-view/kanban-board.tsx (1)
14-52: Props interface is comprehensive but consider grouping related callbacks.The interface has many callback props (
onEdit,onDelete,onViewOutput, etc.). While this is functional, consider whether some of these could be grouped into a single handler object for better organization, e.g.,featureActions: { edit, delete, viewOutput, ... }.This is optional and may be out of scope for this PR.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/app/server-bundle/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
apps/app/server-bundle/package.json(1 hunks)apps/ui/src/components/layout/sidebar.tsx(1 hunks)apps/ui/src/components/views/board-view/components/kanban-column.tsx(3 hunks)apps/ui/src/components/views/board-view/kanban-board.tsx(7 hunks)apps/ui/src/hooks/use-responsive-kanban.ts(3 hunks)apps/ui/src/main.ts(16 hunks)apps/ui/src/preload.ts(1 hunks)apps/ui/tests/kanban-responsive-scaling.spec.ts(6 hunks)libs/types/src/settings.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/ui/src/components/views/board-view/kanban-board.tsx (1)
apps/ui/src/hooks/use-responsive-kanban.ts (1)
useResponsiveKanban(48-194)
apps/ui/tests/kanban-responsive-scaling.spec.ts (1)
apps/ui/tests/utils/git/worktree.ts (3)
createTempDirPath(52-55)setupProjectWithPathNoWorktrees(362-404)waitForBoardView(467-487)
apps/ui/src/hooks/use-responsive-kanban.ts (1)
apps/ui/src/store/app-store.ts (1)
useAppStore(838-2265)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
🔇 Additional comments (23)
apps/ui/src/preload.ts (1)
41-43: LGTM!The new
updateMinWidthAPI is correctly exposed, matching the IPC handler signature inmain.ts. The method appropriately returnsPromise<void>and uses a clear parameter name.libs/types/src/settings.ts (2)
73-90: LGTM!The
WindowBoundsinterface is well-designed with clear JSDoc documentation. All required fields for window state persistence are included.
294-297: LGTM!Making
windowBoundsoptional is the correct approach since this is an Electron-only feature. The documentation clearly explains its purpose.apps/ui/src/components/views/board-view/components/kanban-column.tsx (1)
41-46: LGTM!Good optimization to narrow the transition to only
box-shadowandringproperties. This prevents the width from animating during responsive resizing, which would cause visual jank. The comment clearly explains the rationale.apps/ui/tests/kanban-responsive-scaling.spec.ts (2)
206-244: LGTM! Good test coverage for the minimum width constraint.This test correctly validates that the kanban layout fits within the new Electron minimum width (1500px) without horizontal scrolling. The column width bounds (280-360px) align with the constants defined in
main.ts.
246-292: LGTM! Thorough test for sidebar collapse behavior.The test correctly handles the conditional visibility of the collapse button and verifies that:
- Columns scale appropriately when more space is available
- Width stays within expected bounds
- No horizontal scrollbar appears after collapse
apps/ui/src/main.ts (5)
34-53: LGTM! Well-documented sizing constants.The calculations are clear and the comments explain how the values derive from the kanban layout requirements. The breakdown (columns, gaps, padding, sidebar) makes it easy to verify correctness.
137-162: LGTM! Good debounce implementation for window bounds persistence.The 500ms debounce prevents excessive disk writes during window drag/resize operations. Using
getNormalBounds()for maximized windows correctly captures the pre-maximized dimensions.
164-204: Solid display validation logic.The implementation correctly:
- Checks if the window center is visible on any display
- Resets to primary display if completely off-screen
- Constrains dimensions to fit within the display
- Enforces minimum dimensions
442-469: LGTM! Proper window state persistence lifecycle.The implementation correctly:
- Saves immediately on
close(not debounced) to capture final state- Uses debounced saves for
resizedandmovedevents- Handles both maximized and normal window states
620-631: LGTM! Dynamic minimum width adjustment.The IPC handler correctly updates the window's minimum size based on sidebar state and resizes the window if it's below the new minimum. This ensures the kanban board always fits.
apps/app/server-bundle/package.json (1)
9-9: No action needed on dotenv version.The specified version
^17.2.3is correct—it is the latest version of dotenv. The original concern was based on outdated information; dotenv has progressed beyond the 16.x series.apps/ui/src/hooks/use-responsive-kanban.ts (6)
1-2: LGTM!Import changes are appropriate:
useLayoutEffectfor pre-paint calculations,useReffor mutable timeout reference, anduseAppStorefor sidebar state access.
15-24: Config changes look intentional, but verifyInfinitybehavior.Setting
columnMaxWidth: Infinityallows columns to scale without upper bound. This is fine for the clamping logic sinceMath.min(Infinity, x) = x, but ensure this is the intended behavior for all viewport sizes—very wide monitors will have very wide columns.
95-110: Good use ofuseLayoutEffectfor synchronous measurement.Using
useLayoutEffectto calculate width before paint correctly prevents the visual "bounce" effect. TheisInitializedstate flag is a useful addition for consumers to know when the initial calculation is complete.
112-159: ResizeObserver setup is solid with proper cleanup.The implementation correctly:
- Creates and disconnects the observer in cleanup
- Clears the debounce timeout on unmount
- Falls back to window resize when ResizeObserver isn't available
One minor observation: the ResizeObserver callback updates state directly (not debounced), while the window resize fallback is debounced. This asymmetry is intentional since ResizeObserver is more efficient, but worth noting for consistency.
161-169: Sidebar transition timing uses magic number offset.The
+ 50offset added toSIDEBAR_TRANSITION_MSis a common pattern but can be fragile if the sidebar animation timing changes. Consider documenting why the offset exists or making it a named constant.Additionally, this effect creates a new timeout on every
sidebarOpentoggle, which is correct behavior for re-triggering after transitions.
174-185:totalBoardWidthcalculation is consistent with column width logic.The formula correctly computes the total width including gaps. The
containerStyleusesjustifyContent: 'center'to center columns, which works well with the flex layout.apps/ui/src/components/views/board-view/kanban-board.tsx (5)
1-12: LGTM!Import statements are well-organized. The quote normalization (to single quotes) appears to be consistent with codebase conventions.
85-97: Good integration of responsive hook with container styling.The comment accurately describes the purpose. Using
overflow-x-hiddenon the outer container combined with the hook'scontainerStyle(which includesjustifyContent: 'center') effectively prevents horizontal scrolling while centering the columns.Note:
overflow-x-hiddenwill clip any content that extends beyond the container bounds. Ensure this is acceptable for all column configurations.
112-158: Column-specific header actions are correctly implemented.The conditional rendering for
headerActionbased oncolumn.idis clear:
'verified'column shows "Archive All" when features exist'backlog'column shows suggestions button and conditional "Make" buttonThe logic is straightforward and readable.
165-170: Shortcut key assignment logic is correct.The calculation assigns keys
'1'-'9'for indices 0-8 and'0'for index 9, providing keyboard shortcuts for the first 10 in-progress cards.
204-225: DragOverlay implementation looks good.The easing function and drop animation configuration are appropriate for smooth drag-and-drop UX. The overlay card correctly uses
columnWidthfor consistent sizing.
|
Hey @trueheads, I noticed we both tackled the Kanban scaling issue. I submitted a targeted fix for #85 in #200 earlier and left a question. I see you've gone for a broader rebuild. |
|
Hey @illia1f, I had a chat with some of the other core contributors and decided to just rebuild the scaling logic pretty much entirely. This update will fix the scaling logic for both the electron window scaling, and the kanban scaling for web/electron. This should be a solid/stable approach that allows the app to also preserve session-to-session as well. I did not choose to merge your PR because we were just throwing more code at an originally bad design (my own), so I took an hour and rebuilt/rethought how this should work. I have not had time to fix the merge conflicts, but until I do, feel free to clone this branch and test it out. |
|
Resolves merge conflicts while preserving: - Kanban scaling improvements (window sizing, bounce prevention, debouncing) - Main's sidebar refactoring into hooks - Main's openInEditor functionality for VS Code integration 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/ui/src/components/layout/sidebar/hooks/use-sidebar-auto-collapse.ts (1)
39-43: Consider simplifying the type assertion.The inline type assertion for
window.electronAPIis verbose. For better maintainability, consider declaring a global type definition or extracting this to a typed helper function.🔎 Alternative approach with global type declaration
Create a type definition file (e.g.,
types/electron.d.ts):declare global { interface Window { electronAPI?: { updateMinWidth?: (expanded: boolean) => Promise<void>; // ... other electron API methods }; } } export {};Then simplify the code to:
useEffect(() => { - const electronAPI = ( - window as unknown as { - electronAPI?: { updateMinWidth?: (expanded: boolean) => Promise<void> }; - } - ).electronAPI; + const electronAPI = window.electronAPI; if (electronAPI?.updateMinWidth) { electronAPI.updateMinWidth(sidebarOpen); } }, [sidebarOpen]);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/ui/src/components/layout/sidebar.tsxapps/ui/src/components/layout/sidebar/hooks/use-sidebar-auto-collapse.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
🔇 Additional comments (1)
apps/ui/src/components/layout/sidebar.tsx (1)
124-125: LGTM! Comment accurately reflects the extended behavior.The updated comment correctly documents that
useSidebarAutoCollapsenow handles both auto-collapse on small screens and Electron window minWidth synchronization.
| // Update Electron window minWidth when sidebar state changes | ||
| // This ensures the window can't be resized smaller than what the kanban board needs | ||
| useEffect(() => { | ||
| const electronAPI = ( | ||
| window as unknown as { | ||
| electronAPI?: { updateMinWidth?: (expanded: boolean) => Promise<void> }; | ||
| } | ||
| ).electronAPI; | ||
| if (electronAPI?.updateMinWidth) { | ||
| electronAPI.updateMinWidth(sidebarOpen); | ||
| } | ||
| }, [sidebarOpen]); |
There was a problem hiding this comment.
Add error handling for the async Electron API call.
The updateMinWidth call returns a promise that is not awaited and lacks error handling. If the IPC call fails or rejects, it will result in an unhandled promise rejection.
Consider adding a .catch() handler to log errors gracefully:
🔎 Proposed fix with error handling
// Update Electron window minWidth when sidebar state changes
// This ensures the window can't be resized smaller than what the kanban board needs
useEffect(() => {
const electronAPI = (
window as unknown as {
electronAPI?: { updateMinWidth?: (expanded: boolean) => Promise<void> };
}
).electronAPI;
if (electronAPI?.updateMinWidth) {
- electronAPI.updateMinWidth(sidebarOpen);
+ electronAPI.updateMinWidth(sidebarOpen).catch((error) => {
+ console.error('[useSidebarAutoCollapse] Failed to update window minWidth:', error);
+ });
}
}, [sidebarOpen]);🤖 Prompt for AI Agents
In apps/ui/src/components/layout/sidebar/hooks/use-sidebar-auto-collapse.ts
around lines 36 to 47, the call to the async Electron IPC method updateMinWidth
is not awaited and has no error handling, which can produce unhandled promise
rejections; wrap the call so rejections are handled — either invoke it inside an
async IIFE with try/catch or append .catch() to the returned promise and log the
error (use the app logger if available, otherwise console.error) to fail
gracefully.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/tests/unit/lib/model-resolver.test.ts (1)
35-41: Incomplete refactoring: Line 37 should use CLAUDE_MODEL_MAP.opus.While this file successfully refactors most Claude model references to use
CLAUDE_MODEL_MAP(lines 27, 32, 53, 96, 100, 112), line 37 still contains the hardcoded string'claude-opus-4-5-20251101'. This inconsistency creates a maintenance burden and contradicts the pattern established throughout the rest of the file.🔎 Suggested fix to complete the refactoring
it("should resolve 'opus' alias to full model string", () => { const result = resolveModelString('opus'); - expect(result).toBe('claude-opus-4-5-20251101'); + expect(result).toBe(CLAUDE_MODEL_MAP.opus); expect(consoleSpy.log).toHaveBeenCalledWith( expect.stringContaining('Resolved model alias: "opus"') ); });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/server/tests/unit/lib/model-resolver.test.tsapps/server/tests/unit/providers/claude-provider.test.tsapps/ui/tests/context-view.spec.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/server/tests/unit/lib/model-resolver.test.ts (1)
libs/types/src/index.ts (1)
CLAUDE_MODEL_MAP(36-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
🔇 Additional comments (7)
apps/ui/tests/context-view.spec.ts (5)
49-67: LGTM!The refactoring from the "Add File" flow to the dedicated "Create Markdown" flow is clean and consistent. Testids are properly updated, and the test logic correctly validates the new dialog lifecycle.
362-395: LGTM!The drag-and-drop test is properly updated to use the new Create Markdown flow. The test correctly verifies that dropping a
.mdfile auto-populates both the content textarea and filename field.
470-482: LGTM!The duplicate filename test correctly uses the new Create Markdown flow and properly validates the overwrite behavior.
514-545: LGTM!Both file creation sequences are properly updated to the new Create Markdown flow. The test effectively validates special character handling (parentheses, hyphens, underscores) in filenames.
576-586: LGTM!The empty content edge case test is properly updated. The test correctly validates that files can be created with no content and that subsequent saves with empty content work as expected.
apps/server/tests/unit/providers/claude-provider.test.ts (1)
1-406: These test changes appear unrelated to the PR's stated objective.This PR is titled "Kanban scaling" and described as a rebuild of Kanban scaling logic for Electron window and web behavior. However, this file contains only Claude AI model test updates, which have no apparent connection to Kanban UI scaling.
Please clarify:
- Are these changes intentionally included (e.g., from a merge)?
- Should these be in a separate PR focused on Claude model updates?
- If intentional, how do these model changes relate to Kanban scaling?
apps/server/tests/unit/lib/model-resolver.test.ts (1)
1-139: These test changes appear unrelated to the PR's stated objective.This PR is titled "Kanban scaling" and aims to rebuild Kanban scaling logic for Electron and web. However, this file only refactors Claude AI model test references to use centralized constants—changes that have no connection to UI scaling or Kanban functionality.
Please clarify whether these changes were intentionally included or should be split into a separate PR focused on model constant refactoring.
| it('should include Claude Haiku 4.5', () => { | ||
| const models = provider.getAvailableModels(); | ||
|
|
||
| const haiku = models.find((m) => m.id === 'claude-3-5-haiku-20241022'); | ||
| const haiku = models.find((m) => m.id === 'claude-haiku-4-5-20251001'); | ||
| expect(haiku).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for CLAUDE_MODEL_MAP definition and usage
echo "=== Searching for CLAUDE_MODEL_MAP definition ==="
rg "CLAUDE_MODEL_MAP" --type ts -B 2 -A 5
echo -e "\n=== Checking for model-resolver.test.ts ==="
find . -name "model-resolver.test.ts" -type f
echo -e "\n=== Checking for claude-provider.test.ts ==="
find . -name "claude-provider.test.ts" -type fRepository: AutoMaker-Org/automaker
Length of output: 24947
🏁 Script executed:
#!/bin/bash
echo "=== Checking claude-provider.test.ts around lines 310-315 ==="
sed -n '305,320p' apps/server/tests/unit/providers/claude-provider.test.ts
echo -e "\n=== Searching for hardcoded Claude model strings in claude-provider.test.ts ==="
rg "claude-(opus|sonnet|haiku)" apps/server/tests/unit/providers/claude-provider.test.ts -n
echo -e "\n=== Checking for CLAUDE_MODEL_MAP import in claude-provider.test.ts ==="
head -20 apps/server/tests/unit/providers/claude-provider.test.tsRepository: AutoMaker-Org/automaker
Length of output: 1881
Use CLAUDE_MODEL_MAP for consistency across Claude provider tests.
This test hardcodes 'claude-haiku-4-5-20251001' while the related file model-resolver.test.ts in this same PR adopts CLAUDE_MODEL_MAP constants for all Claude model references, creating maintenance inconsistency. Additionally, line 298 uses an outdated Sonnet model version ('claude-sonnet-4-20250514') instead of the centralized constant.
Refactor lines 59, 71, 289, 298, 313, and 320 to import and use CLAUDE_MODEL_MAP from @automaker/types:
+import { CLAUDE_MODEL_MAP } from '@automaker/types';Then replace hardcoded strings with:
CLAUDE_MODEL_MAP.opus(for'claude-opus-4-5-20251101')CLAUDE_MODEL_MAP.sonnet(for the outdated'claude-sonnet-4-20250514'at line 298)CLAUDE_MODEL_MAP.haiku(for'claude-haiku-4-5-20251001')
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/server/tests/unit/providers/claude-provider.test.ts around lines 59, 71,
289, 298, 313 and 320, replace hardcoded Claude model ID strings with the
centralized CLAUDE_MODEL_MAP constants to keep tests consistent: add an import
of CLAUDE_MODEL_MAP from '@automaker/types' at the top of the file, then replace
occurrences as follows — use CLAUDE_MODEL_MAP.opus where the test currently uses
'claude-opus-4-5-20251101', use CLAUDE_MODEL_MAP.sonnet where it uses
'claude-sonnet-4-20250514' (line 298), and use CLAUDE_MODEL_MAP.haiku where it
uses 'claude-haiku-4-5-20251001' (lines 313/320); ensure all referenced test
assertions and finds use these constants instead of string literals.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/ui/src/components/views/context-view.tsx (1)
1003-1029: Consider extracting the drag-and-drop handler.The inline
onDrophandler spans 23 lines, which makes the JSX less readable. Extracting it to a separate function would improve maintainability.🔎 Suggested refactor
Define the handler before the return statement:
// Handle markdown file drop in Create Markdown dialog const handleMarkdownDrop = useCallback( async (e: React.DragEvent<HTMLTextAreaElement>) => { e.preventDefault(); e.stopPropagation(); let files = Array.from(e.dataTransfer.files); if (files.length === 0 && e.dataTransfer.items) { const items = Array.from(e.dataTransfer.items); files = items .filter((item) => item.kind === 'file') .map((item) => item.getAsFile()) .filter((f): f is globalThis.File => f !== null); } const mdFile = files.find((f) => isMarkdownFile(f.name)); if (mdFile) { try { const content = await mdFile.text(); setNewMarkdownContent(content); if (!newMarkdownName.trim()) { setNewMarkdownName(mdFile.name); } } catch (error) { console.error('Failed to read dropped file:', error); toast.error('Failed to read file', { description: 'The dropped file could not be read.', }); } } }, [isMarkdownFile, newMarkdownName] );Then in JSX:
<textarea id="markdown-content" value={newMarkdownContent} onChange={(e) => setNewMarkdownContent(e.target.value)} - onDrop={async (e) => { /* ... 23 lines ... */ }} + onDrop={handleMarkdownDrop} onDragOver={(e) => { e.preventDefault(); e.stopPropagation(); }}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/ui/src/components/views/context-view.tsxapps/ui/tests/utils/files/drag-drop.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
🔇 Additional comments (1)
apps/ui/tests/utils/files/drag-drop.ts (1)
22-44: LGTM! Excellent improvement for cross-browser compatibility.The refactored approach using
Object.definePropertyto explicitly define thedataTransferproperty ensures it's accessible across all browsers. Some browsers don't properly exposedataTransferwhen passed via the event constructor, so this pattern is a best practice for test utilities.The added comment clearly documents the rationale, making future maintenance easier.
| onDrop={async (e) => { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
|
|
||
| // Try files first, then items for better compatibility | ||
| let files = Array.from(e.dataTransfer.files); | ||
| if (files.length === 0 && e.dataTransfer.items) { | ||
| const items = Array.from(e.dataTransfer.items); | ||
| files = items | ||
| .filter((item) => item.kind === 'file') | ||
| .map((item) => item.getAsFile()) | ||
| .filter((f): f is globalThis.File => f !== null); | ||
| } | ||
|
|
||
| const mdFile = files.find((f) => isMarkdownFile(f.name)); | ||
| if (mdFile) { | ||
| const content = await mdFile.text(); | ||
| setNewMarkdownContent(content); | ||
| if (!newMarkdownName.trim()) { | ||
| setNewMarkdownName(mdFile.name); | ||
| } | ||
| } | ||
| }} |
There was a problem hiding this comment.
Add error handling for file reading.
The async mdFile.text() call on line 1019 can throw an error if the file is unreadable or corrupted. Currently, any error would silently fail and leave the user without feedback.
🔎 Proposed fix with error handling
const mdFile = files.find((f) => isMarkdownFile(f.name));
if (mdFile) {
- const content = await mdFile.text();
- setNewMarkdownContent(content);
- if (!newMarkdownName.trim()) {
- setNewMarkdownName(mdFile.name);
+ try {
+ const content = await mdFile.text();
+ setNewMarkdownContent(content);
+ if (!newMarkdownName.trim()) {
+ setNewMarkdownName(mdFile.name);
+ }
+ } catch (error) {
+ console.error('Failed to read dropped file:', error);
+ toast.error('Failed to read file', {
+ description: 'The dropped file could not be read.',
+ });
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/ui/src/components/views/context-view.tsx around lines 1003 to 1025, the
async mdFile.text() call can throw if the file is unreadable; wrap the file read
in a try/catch, log the error (console.error) and surface user feedback (e.g.,
set an error state or call an existing toast/notification helper) so the UI
informs the user the file could not be read; only setNewMarkdownContent and
setNewMarkdownName when reading succeeds, and keep existing behavior for naming
fallback.
3 participants
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.