fix background image and create test incase background image failed again #375
Conversation
# Conflicts: # apps/ui/src/routes/__root.tsx
📝 WalkthroughWalkthroughLoads project-specific UI settings from the server when the current project changes and applies them to the app store via a new React hook; the hook is invoked in the root layout and covered by an end-to-end Playwright test that verifies persistence across project switches and restarts. (50 words) Changes
Sequence DiagramsequenceDiagram
participant App as Root Layout
participant Hook as useProjectSettingsLoader
participant Store as App Store
participant API as HTTP API Server
App->>Hook: invoke on mount
Store->>Hook: currentProject.path changes
Hook->>Hook: skip if lastLoadedRef == path
rect rgb(220,235,255)
Hook->>API: GET /api/settings/project?path=currentProject.path
API-->>Hook: 200 { project settings }
end
rect rgb(200,235,200)
Hook->>Store: setBoardBackground(...)
Hook->>Store: setCardOpacity(...)
Hook->>Store: setColumnOpacity(...)
Hook->>Store: setColumnBorderEnabled(...)
Hook->>Store: setCardGlassmorphism(...)
Hook->>Store: setCardBorderEnabled(...)
Hook->>Store: setCardBorderOpacity(...)
Hook->>Store: setHideScrollbar(...)
end
alt API error
API--xHook: error
Hook->>Hook: console.error(...)
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⏰ 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)
🔇 Additional comments (4)
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 |
Summary of ChangesHello @yumesha, 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 crucial fix for the inconsistent loading of board background images and associated project settings. By implementing a dedicated React hook that proactively retrieves and applies these settings within the root layout, the application now ensures a seamless and persistent display of user-configured board aesthetics. The changes are thoroughly validated with new end-to-end tests, guaranteeing the reliability of this functionality. 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
|
![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 effectively addresses the background image loading issue by introducing a dedicated hook, useProjectSettingsLoader, and adding a comprehensive E2E test to prevent future regressions. The approach is solid. My review includes suggestions to improve the maintainability of the new hook by reducing code repetition and to enhance the robustness of the new tests by replacing fixed timeouts with more reliable event-based waits.
| if (result.success && result.settings?.boardBackground) { | ||
| const bg = result.settings.boardBackground; | ||
|
|
||
| // Update store with loaded settings (without triggering server save) | ||
| setBoardBackground(currentProject.path, bg.imagePath); | ||
|
|
||
| if (bg.cardOpacity !== undefined) { | ||
| setCardOpacity(currentProject.path, bg.cardOpacity); | ||
| } | ||
|
|
||
| if (bg.columnOpacity !== undefined) { | ||
| setColumnOpacity(currentProject.path, bg.columnOpacity); | ||
| } | ||
|
|
||
| if (bg.columnBorderEnabled !== undefined) { | ||
| setColumnBorderEnabled(currentProject.path, bg.columnBorderEnabled); | ||
| } | ||
|
|
||
| if (bg.cardGlassmorphism !== undefined) { | ||
| setCardGlassmorphism(currentProject.path, bg.cardGlassmorphism); | ||
| } | ||
|
|
||
| if (bg.cardBorderEnabled !== undefined) { | ||
| setCardBorderEnabled(currentProject.path, bg.cardBorderEnabled); | ||
| } | ||
|
|
||
| if (bg.cardBorderOpacity !== undefined) { | ||
| setCardBorderOpacity(currentProject.path, bg.cardBorderOpacity); | ||
| } | ||
|
|
||
| if (bg.hideScrollbar !== undefined) { | ||
| setHideScrollbar(currentProject.path, bg.hideScrollbar); | ||
| } | ||
| } |
There was a problem hiding this comment.
The series of if statements to update the store is quite repetitive. This can be refactored into a more concise and maintainable structure using a map of setting keys to their corresponding setter functions. This approach will make it easier to add or remove settings in the future, as you would only need to update the map.
if (result.success && result.settings?.boardBackground) {
const bg = result.settings.boardBackground;
const projectPath = currentProject.path;
// Update store with loaded settings (without triggering server save)
setBoardBackground(projectPath, bg.imagePath);
const settingsMap = {
cardOpacity: setCardOpacity,
columnOpacity: setColumnOpacity,
columnBorderEnabled: setColumnBorderEnabled,
cardGlassmorphism: setCardGlassmorphism,
cardBorderEnabled: setCardBorderEnabled,
cardBorderOpacity: setCardBorderOpacity,
hideScrollbar: setHideScrollbar,
} as const;
for (const [key, setter] of Object.entries(settingsMap)) {
const value = bg[key as keyof typeof bg];
if (value !== undefined) {
// The type assertion is safe because we constructed the map carefully.
(setter as (path: string, val: typeof value) => void)(projectPath, value);
}
}
}|
|
||
| // CRITICAL: Wait for settings to be loaded (useProjectSettingsLoader hook) | ||
| // This ensures the background settings are fetched from the server | ||
| await page.waitForTimeout(2000); |
There was a problem hiding this comment.
Using page.waitForTimeout() can lead to flaky tests because it waits for a fixed amount of time, which may not be enough on a slow machine or too long on a fast one. It's more robust to wait for a specific event to occur. In this case, you can wait for the network request that loads the project settings to complete.
| await page.waitForTimeout(2000); | |
| await page.waitForResponse(resp => | |
| resp.url().includes('/api/settings/project') && | |
| resp.request().postData()?.includes(projectAPath) | |
| ); |
| ).toBeVisible({ timeout: 5000 }); | ||
|
|
||
| // CRITICAL: Wait for settings to be loaded again | ||
| await page.waitForTimeout(2000); |
There was a problem hiding this comment.
Similar to the previous wait, using page.waitForResponse() is more reliable than page.waitForTimeout(). This will ensure the settings have been reloaded after switching back to the project before the test proceeds, making the test less prone to race conditions.
await page.waitForResponse(resp =>
resp.url().includes('/api/settings/project') &&
resp.request().postData()?.includes(projectAPath)
);| await expect(page.locator('[data-testid="board-view"]')).toBeVisible({ timeout: 15000 }); | ||
|
|
||
| // Wait for settings to load | ||
| await page.waitForTimeout(2000); |
There was a problem hiding this comment.
To make this test more robust and avoid potential flakiness, it's better to wait for the specific network request to complete rather than using a fixed timeout. page.waitForResponse() can be used to wait for the settings to be loaded from the API.
await page.waitForResponse(resp =>
resp.url().includes('/api/settings/project') &&
resp.request().postData()?.includes(projectPath)
);![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI Agents
In @apps/ui/src/hooks/use-project-settings-loader.ts:
- Line 40: The current check in useProjectSettingsLoader only continues when
result.success && result.settings?.boardBackground, which skips valid settings
objects that lack boardBackground; change the condition to check result.success
&& result.settings so settings are processed regardless of boardBackground
presence, and then handle boardBackground as an optional property (e.g., apply
it only if result.settings.boardBackground exists or use a default) while
keeping the rest of the settings application logic unchanged.
- Around line 81-91: The effect in use-project-settings-loader (the useEffect
that depends on currentProject?.path and many setters like setBoardBackground,
setCardOpacity, setColumnOpacity, setColumnBorderEnabled, setCardGlassmorphism,
setCardBorderEnabled, setCardBorderOpacity, setHideScrollbar) should only depend
on currentProject?.path; remove all the stable setter functions from the
dependency array and change it to only [currentProject?.path], keeping the
existing loadingRef guard and effect body unchanged so the effect triggers only
when the project path changes.
In @apps/ui/tests/projects/board-background-persistence.spec.ts:
- Line 252: The test is flaky because the hook uses loadingRef to prevent
duplicate loads, so change the assertion on projectASettingsCalls from
expect(projectASettingsCalls.length).toBeGreaterThanOrEqual(2) to a more lenient
expect(projectASettingsCalls.length).toBeGreaterThanOrEqual(1); alternatively,
if you want to assert the second load, update the test to simulate clearing the
hook's loadingRef by unmounting/remounting or explicitly switching projects so
the hook (loadingRef) is reset before switching back — locate the test variables
projectASettingsCalls and the hook's loadingRef usage to implement the change.
🧹 Nitpick comments (2)
apps/ui/tests/projects/board-background-persistence.spec.ts (2)
189-189: Replace hardcoded timeouts with deterministic waits.The test uses multiple
page.waitForTimeout()calls with arbitrary delays (2000ms, 500ms). This is an anti-pattern that makes tests slower and potentially flaky. Instead, wait for specific DOM conditions or API responses.🔎 Recommended approach to replace timeouts
Instead of:
await page.waitForTimeout(2000);Consider waiting for the API call to complete:
// Wait for settings API call to complete const settingsLoadPromise = page.waitForRequest( request => request.url().includes('/api/settings/project') && request.method() === 'POST' && (request.postData()?.includes(projectAPath) ?? false), { timeout: 5000 } ); await settingsLoadPromise;Or wait for a DOM element that indicates settings are loaded (if such an element exists):
// Wait for background to be applied (if there's a CSS property or data attribute you can check) await expect(boardView).toHaveAttribute('data-background-loaded', 'true', { timeout: 5000 });Also applies to: 238-238, 373-373
192-194: Missing verification of actual UI rendering.The comment acknowledges that the test can't directly access React state and should verify via DOM/CSS, but then doesn't actually perform this verification. The test only confirms that API calls are made and the settings file has correct content, but doesn't verify that the background image is visually applied.
Consider adding a check for the background image in the DOM:
// Check if the background image is applied via inline style or CSS const boardView = page.locator('[data-testid="board-view"]'); const backgroundStyle = await boardView.evaluate((el) => { return window.getComputedStyle(el).backgroundImage; }); expect(backgroundStyle).toContain('background.jpg');
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/ui/tests/img/background.jpgis excluded by!**/*.jpg
📒 Files selected for processing (3)
apps/ui/src/hooks/use-project-settings-loader.tsapps/ui/src/routes/__root.tsxapps/ui/tests/projects/board-background-persistence.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/hooks/use-project-settings-loader.tsapps/ui/tests/projects/board-background-persistence.spec.tsapps/ui/src/routes/__root.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/hooks/use-project-settings-loader.tsapps/ui/tests/projects/board-background-persistence.spec.tsapps/ui/src/routes/__root.tsx
🧠 Learnings (2)
📚 Learning: 2025-12-28T05:07:48.147Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.147Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Always import from shared packages (automaker/*), never from old relative paths
Applied to files:
apps/ui/src/routes/__root.tsx
📚 Learning: 2025-12-30T01:02:07.114Z
Learnt from: illia1f
Repo: AutoMaker-Org/automaker PR: 324
File: apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx:122-131
Timestamp: 2025-12-30T01:02:07.114Z
Learning: Tailwind CSS v4 uses postfix syntax for the important modifier: append ! at the end of the utility class (e.g., backdrop-blur-[0px]! or hover:bg-red-500!). The older v3 style used a prefix (!) at the start (e.g., !backdrop-blur-[0px]); prefer the postfix form for consistency across TSX files.
Applied to files:
apps/ui/src/routes/__root.tsx
🧬 Code graph analysis (3)
apps/ui/src/hooks/use-project-settings-loader.ts (1)
apps/ui/src/lib/http-api-client.ts (1)
getHttpApiClient(1985-1990)
apps/ui/tests/projects/board-background-persistence.spec.ts (3)
apps/ui/tests/utils/git/worktree.ts (1)
createTempDirPath(52-55)apps/ui/tests/utils/api/client.ts (1)
authenticateForTests(357-361)apps/ui/tests/utils/core/interactions.ts (1)
handleLoginScreenIfPresent(65-115)
apps/ui/src/routes/__root.tsx (1)
apps/ui/src/hooks/use-project-settings-loader.ts (1)
useProjectSettingsLoader(10-92)
⏰ 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 (2)
apps/ui/src/routes/__root.tsx (1)
27-27: LGTM! Hook integration is clean and well-placed.The
useProjectSettingsLoaderhook is correctly imported and invoked withinRootLayoutContent. The placement is appropriate since the hook has internal guards (!currentProject?.path) that prevent premature execution before the app is fully initialized.Also applies to: 51-52
apps/ui/tests/projects/board-background-persistence.spec.ts (1)
41-266: Good test coverage for the loading mechanism.The tests effectively verify that:
- Settings are loaded when switching between projects
- Settings are loaded on app restart with a current project
- API calls are made with the correct project paths
- Settings persist in the file system
This provides solid coverage for the
useProjectSettingsLoaderhook functionality, even though UI rendering verification is missing.Also applies to: 268-398
| console.log('Looking for path:', projectAPath); | ||
| } | ||
|
|
||
| expect(projectASettingsCalls.length).toBeGreaterThanOrEqual(2); |
There was a problem hiding this comment.
Test expectation may be flaky due to duplicate load prevention.
The test expects at least 2 API calls for project A (initial load + switch back), but the loadingRef in the hook prevents duplicate loads for the same project path. If the ref isn't cleared when switching away from project A, the second load might be skipped, causing this assertion to fail.
Consider:
- Verifying the hook clears the ref when the project changes (it doesn't currently)
- Adjusting the expectation to be more lenient:
expect(projectASettingsCalls.length).toBeGreaterThanOrEqual(1) - Adding logic to track project switches and verify the behavior more precisely
🤖 Prompt for AI Agents
In @apps/ui/tests/projects/board-background-persistence.spec.ts at line 252, The
test is flaky because the hook uses loadingRef to prevent duplicate loads, so
change the assertion on projectASettingsCalls from
expect(projectASettingsCalls.length).toBeGreaterThanOrEqual(2) to a more lenient
expect(projectASettingsCalls.length).toBeGreaterThanOrEqual(1); alternatively,
if you want to assert the second load, update the test to simulate clearing the
hook's loadingRef by unmounting/remounting or explicitly switching projects so
the hook (loadingRef) is reset before switching back — locate the test variables
projectASettingsCalls and the hook's loadingRef usage to implement the change.
|
@claude address the pr comments |
|
can you address the comments and see if they are valid |
|
@webdevcody I have address the PR it all good now. |
2 participants
Summary
Fixes background image not loading when switching between projects by ensuring project settings are properly loaded on mount.
Problem
Background images were not displaying correctly due to missing project settings initialization in the root layout. The E2E test
board-background-persistence.spec.tswas failing because theuseProjectSettingsLoaderhook wasn't being invoked.Solution
useProjectSettingsLoader()hook to root layout component (apps/ui/src/routes/__root.tsx)Changes
apps/ui/src/routes/__root.tsx: Import and invokeuseProjectSettingsLoader()hookTesting
board-background-persistence.spec.tsverifies background loads on project switchSummary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.