feat(projects): mobile shell — responsive layout, swipe board, mobile task modal#271
Merged
feat(projects): mobile shell — responsive layout, swipe board, mobile task modal#271
Conversation
Address two Important findings from the Task 3 code-quality review: - Rename JSX variables `list`/`detail` to `listPane`/`detailPane` to avoid shadowing the local `const list` inside `refresh()`. Component-level rename is the lower-churn fix; `refresh()` body is left untouched. `MobileSplitView` prop names (`list`/`detail`) stay as-is since they're the component's public API. - Have the `MobileSplitView` mock surface `listTitle` as `data-list-title` and assert on it in the mobile-path test. This means the test now verifies the production code actually routed through the `MobileSplitView` branch with the expected props, rather than just finding any element wearing the testid.
…ex-col convention
…rojectBoard mobile branch
…lways-on group-by
…+ status branch tests + close autofocus
…er vertical-overshoot
Uses Approach B: editor.updateInstanceState({ isReadonly: isMobile }) inside
onMount, plus a separate useEffect that re-applies the state whenever isMobile
changes (e.g. window resize crossing the 768px breakpoint).
The <Tldraw> component in tldraw v4 (@tldraw/tldraw ^4.5.10) does NOT expose
a readOnly prop — readonly mode is exclusively controlled via the editor API.
Confirmed via tldraw.dev official docs (sdk-features/readonly and
examples/readonly). The existing onMount store.listen() listener for
user-edit forwarding is preserved unchanged.
📝 WalkthroughWalkthroughAdds comprehensive mobile support and E2E/tests to the Projects app: new mobile UI components (carousel board, task modal, FAB, sheet, tab pills), responsive edits to existing components, task creation/refresh event plumbing, Playwright config and scripts, and many mobile-focused unit and E2E tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant FAB as ProjectFab
participant Sheet as TaskCreateSheet
participant Workspace as ProjectWorkspace
participant API as projectsApi
participant Window
User->>FAB: click "Create task"
FAB->>Workspace: open sheet
Workspace->>Sheet: open=true
User->>Sheet: submit title
Sheet->>API: tasks.create(projectId, { title })
API-->>Sheet: created task
Sheet->>Window: dispatch 'projects:tasks-refresh' { projectId }
Window->>Workspace: event captured
Workspace->>API: refresh tasks (via ProjectTaskList)
API-->>Workspace: updated task list
Workspace-->>User: UI updated, sheet closed
sequenceDiagram
actor User
participant Carousel as MobileBoardCarousel
participant ModalWrapper as TaskModal
participant MobileModal as MobileTaskModal
participant API as projectsApi
participant Window
User->>Carousel: tap task card
Carousel->>ModalWrapper: onOpenTask(taskId)
ModalWrapper->>MobileModal: render mobile modal
User->>MobileModal: tap status action (e.g., "Claim")
MobileModal->>ModalWrapper: onChangeStatus(next)
ModalWrapper->>API: tasks.update(taskId, { status: next })
API-->>ModalWrapper: updated task
ModalWrapper->>Window: dispatch 'projects:tasks-refresh'
Window->>Carousel: task list refresh triggers re-render
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 2/3 reviews remaining, refill in 20 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
desktop/src/apps/ProjectsApp/mobile/__tests__/MobileBoardCarousel.test.tsx (1)
84-86: 💤 Low valueUnnecessary aliased import.
The
describe as describe2alias is redundant sincedescribeis already imported on line 1 and can be reused.-import { describe as describe2 } from "vitest"; - -describe2("MobileBoardCarousel — swimlane section headers", () => { +describe("MobileBoardCarousel — swimlane section headers", () => {,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/ProjectsApp/mobile/__tests__/MobileBoardCarousel.test.tsx` around lines 84 - 86, The test imports describe as an alias describe2 and then uses describe2 in MobileBoardCarousel.test; remove the aliased import and replace references to describe2 with the existing describe (i.e., delete the "import { describe as describe2 } from 'vitest';" line and update the describe2("MobileBoardCarousel — swimlane section headers", ...) call to describe(...)) so the file uses the original describe import consistently.desktop/src/apps/ProjectsApp/mobile/MobileBoardCarousel.tsx (1)
31-56: 💤 Low valueConsider clearing
paneRefswhen columns change.If
columnswere to change dynamically,paneRefs.currentcould retain stale references at incorrect indices. SinceMOBILE_COLUMNSis constant inProjectBoard.tsx, this is low risk, but for robustness:export function MobileBoardCarousel({ columns, tasksByColumn, groupBy, onOpenTask }: Props) { const [activeIdx, setActiveIdx] = useState(0); const scrollerRef = useRef<HTMLDivElement>(null); const paneRefs = useRef<Array<HTMLDivElement | null>>([]); + + // Reset refs array when columns change + paneRefs.current.length = columns.length;,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/ProjectsApp/mobile/MobileBoardCarousel.tsx` around lines 31 - 56, paneRefs.current can hold stale DOM references if the columns array changes; add logic to reset or resize paneRefs when columns change by creating a useEffect that watches columns (or columns.length) and sets paneRefs.current = new Array(columns.length). Ensure this update occurs before any scroller interactions (so add the effect alongside the existing scroll effect) and keep references assigned via the existing ref callbacks so paneRefs entries align with the current columns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@desktop/playwright.config.ts`:
- Around line 11-25: The Playwright config currently points at port 5173
(baseURL and webServer.url) which can collide with any local Vite instance;
change to a dedicated E2E port (e.g. 5174) or read from an env var (e.g.
PLAYWRIGHT_E2E_PORT) and update the webServer.command, webServer.url and
use.baseURL to use that port; ensure the webServer.command passes the port env
var to the dev server start command and keep reuseExistingServer behavior as
desired so Playwright no longer silently attaches to unrelated servers.
In `@desktop/src/apps/ProjectsApp/board/ProjectBoard.tsx`:
- Around line 145-154: The mobile branch currently returns only
MobileBoardCarousel and omits the BoardToolbar, so add BoardToolbar above
MobileBoardCarousel in ProjectBoard.tsx; render <BoardToolbar> in the isMobile
branch and pass the same groupBy and its change handler (e.g., onChangeGroupBy /
setGroupBy), current filters and their change handler (e.g., filters /
onChangeFilters or setFilters), and any mobile flag (isMobile or mobileSheet
prop) so the toolbar shows the mobile "Filter / Group" sheet, then keep
MobileBoardCarousel with the existing props (columns/MOBILE_COLUMNS,
tasksByColumn, onOpenTask) underneath.
In `@desktop/src/apps/ProjectsApp/board/TaskModal.tsx`:
- Around line 77-89: The mobile branch currently returns a fullscreen blocker
when task is falsy and can trap users; change the logic in TaskModal.tsx (the
isMobile branch) to distinguish loading vs not-found: treat undefined as
"loading…" and null as "not found" and render a dismissible dialog that includes
the same close affordance as desktop (call the existing onClose handler).
Specifically, update the condition around isMobile and task so that you only
show the spinner when task === undefined, and when task === null render a small
message ("Task not found") plus the header/close button that invokes onClose
(reuse the modal header/close UI used elsewhere in this component).
In `@desktop/src/apps/ProjectsApp/mobile/TaskCreateSheet.tsx`:
- Around line 53-58: In TaskCreateSheet, the backdrop button currently always
calls onClose() even while a create request is in flight; guard the backdrop
dismissal by checking the component's submit state (e.g., isSubmitting or
submitting) and only call onClose when that flag is false: update the backdrop
button's onClick to something like () => { if (!isSubmitting) onClose(); }, add
disabled / aria-disabled when submitting, and ensure the backdrop has
pointer-events disabled (or a CSS class change) while submitting so users cannot
dismiss the sheet mid-request.
- Around line 66-74: The title input in TaskCreateSheet.tsx lacks a stable
accessible name—replace the placeholder-only textbox with an explicitly
associated label: add a <label> element (e.g., "Title" or "Task title") tied to
the input via htmlFor/id or provide an aria-label/aria-labelledby on the input;
update the input (ref inputRef, value title, onChange setTitle) to include
id="task-title" (or similar) and ensure the label uses htmlFor="task-title" so
assistive tech always has a persistent name for the field.
In `@desktop/src/components/mobile/WorkspaceTabPills.tsx`:
- Around line 40-45: The button in WorkspaceTabPills.tsx (the element using
key={t.id}, ref={isActive ? activeRef : undefined}, role="tab",
aria-selected={isActive}, onClick={() => onSelect(t.id)}) needs an explicit type
attribute to avoid accidental form submission; add type="button" to that
<button> element so it behaves as a non-submit button when rendered inside a
form.
- Around line 28-45: The tablist/tab elements lack required ARIA attributes and
keyboard behavior; update the component that renders tabs (the tabs.map callback
using activeRef and onSelect) to either implement the full WAI-ARIA tabs pattern
or convert to a simpler button/segmented-control pattern: if implementing tabs,
add aria-controls on each button pointing to the associated panel id, ensure the
tablist has aria-labelledby, implement roving tabindex (set the active tab
rendered by tabs.map to tabindex="0" and inactive to tabindex="-1"), and add
keyboard handlers on the tab elements to support ArrowLeft/ArrowRight/Home/End
navigation and Space/Enter activation (use onKeyDown alongside onClick and
update focus via activeRef); alternatively, if panels are managed elsewhere,
change roles from role="tablist"/role="tab" to role="group" or leave as plain
buttons and keep existing onSelect.
In `@desktop/tests/projects-mobile.spec.ts`:
- Around line 42-64: The current early-return when project creation times out
(inside the projectCount === 0 branch using the dialogGone boolean) hides a
failed precondition; replace the silent return with an explicit test failure or
skip so CI doesn’t report a false positive. Specifically, in the block that
currently does "if (!dialogGone) { /* ... */ return; }", either throw a clear
Error (e.g. throw new Error('Project creation timed out - cannot continue
test')) or call the test framework’s skip helper, or alternatively seed a
deterministic project before running the rest of the flow; target the dialogGone
handling and the project creation flow (the code that clicks "create project",
fills nameInput and waits for the dialog) to implement the chosen behavior.
---
Nitpick comments:
In `@desktop/src/apps/ProjectsApp/mobile/__tests__/MobileBoardCarousel.test.tsx`:
- Around line 84-86: The test imports describe as an alias describe2 and then
uses describe2 in MobileBoardCarousel.test; remove the aliased import and
replace references to describe2 with the existing describe (i.e., delete the
"import { describe as describe2 } from 'vitest';" line and update the
describe2("MobileBoardCarousel — swimlane section headers", ...) call to
describe(...)) so the file uses the original describe import consistently.
In `@desktop/src/apps/ProjectsApp/mobile/MobileBoardCarousel.tsx`:
- Around line 31-56: paneRefs.current can hold stale DOM references if the
columns array changes; add logic to reset or resize paneRefs when columns change
by creating a useEffect that watches columns (or columns.length) and sets
paneRefs.current = new Array(columns.length). Ensure this update occurs before
any scroller interactions (so add the effect alongside the existing scroll
effect) and keep references assigned via the existing ref callbacks so paneRefs
entries align with the current columns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: abeea415-7af9-4932-9084-805d4c6315d8
⛔ Files ignored due to path filters (1)
desktop/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
desktop/.gitignoredesktop/package.jsondesktop/playwright.config.tsdesktop/src/apps/ProjectsApp/ProjectActivity.tsxdesktop/src/apps/ProjectsApp/ProjectMembers.tsxdesktop/src/apps/ProjectsApp/ProjectTaskList.tsxdesktop/src/apps/ProjectsApp/ProjectWorkspace.tsxdesktop/src/apps/ProjectsApp/__tests__/ProjectWorkspace.mobile.test.tsxdesktop/src/apps/ProjectsApp/__tests__/ProjectsApp.mobile.test.tsxdesktop/src/apps/ProjectsApp/board/BoardToolbar.tsxdesktop/src/apps/ProjectsApp/board/ProjectBoard.tsxdesktop/src/apps/ProjectsApp/board/TaskModal.tsxdesktop/src/apps/ProjectsApp/canvas/CanvasBoard.tsxdesktop/src/apps/ProjectsApp/index.tsxdesktop/src/apps/ProjectsApp/mobile/MobileBoardCarousel.tsxdesktop/src/apps/ProjectsApp/mobile/MobileTaskModal.tsxdesktop/src/apps/ProjectsApp/mobile/ProjectFab.tsxdesktop/src/apps/ProjectsApp/mobile/TaskCreateSheet.tsxdesktop/src/apps/ProjectsApp/mobile/__tests__/MobileBoardCarousel.test.tsxdesktop/src/apps/ProjectsApp/mobile/__tests__/MobileTaskModal.test.tsxdesktop/src/apps/ProjectsApp/mobile/__tests__/ProjectFab.test.tsxdesktop/src/apps/ProjectsApp/mobile/__tests__/TaskCreateSheet.test.tsxdesktop/src/components/mobile/MobileSplitView.tsxdesktop/src/components/mobile/WorkspaceTabPills.tsxdesktop/src/components/mobile/__tests__/WorkspaceTabPills.test.tsxdesktop/tests/projects-mobile.spec.tsdesktop/vitest.setup.ts
Comment on lines
+11
to
+25
| use: { | ||
| baseURL: "http://127.0.0.1:5173", | ||
| trace: "on-first-retry", | ||
| }, | ||
| projects: [ | ||
| { | ||
| name: "iphone-14", | ||
| use: { ...devices["iPhone 14"] }, | ||
| }, | ||
| ], | ||
| webServer: { | ||
| command: "npm run dev", | ||
| url: "http://127.0.0.1:5173", | ||
| reuseExistingServer: !process.env.CI, | ||
| timeout: 60_000, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "playwright.config.ts" -o -name "vite.config.ts" -o -name "vite.config.js" | head -20Repository: jaylfc/tinyagentos
Length of output: 118
🏁 Script executed:
cat desktop/playwright.config.tsRepository: jaylfc/tinyagentos
Length of output: 682
🏁 Script executed:
cat package.json | head -50Repository: jaylfc/tinyagentos
Length of output: 107
🏁 Script executed:
cat desktop/vite.config.tsRepository: jaylfc/tinyagentos
Length of output: 3647
🏁 Script executed:
ls -la desktop/ | grep -E "(package\.json|vite\.config)"Repository: jaylfc/tinyagentos
Length of output: 194
🏁 Script executed:
cat desktop/package.json 2>/dev/null || echo "No desktop/package.json"Repository: jaylfc/tinyagentos
Length of output: 2197
🌐 Web query:
Vite default dev server port
💡 Result:
The default dev server port for Vite is 5173.
Citations:
- 1: https://main.vitejs.dev/config/server-options
- 2: https://vite.dev/config/server-options
- 3: https://www.reddit.com/r/programming/comments/xh1vyr/fun_fact_vites_default_port_is_5173_which_spells/
- 4: How to change port on a vite-react-app?? vitejs/vite#10834
- 5: https://vite.dev/config/
🌐 Web query:
Playwright reuseExistingServer behavior documentation
💡 Result:
The reuseExistingServer option is part of Playwright's webServer configuration in playwright.config.ts. It controls whether Playwright should reuse an already-running server on the specified port or url instead of starting a new one via the command. When true: - If a server is already listening on the port/url, it reuses it without throwing an error or starting the command. - If no server is running, it starts one using the command. - Commonly set to !process.env.CI to enable locally (reuse dev server for faster iteration) and disable in CI (start fresh for reliability). When false (default in CI setups): - Throws an error if something is already listening on the port/url. - Always starts a new server via command if none exists. Example from official docs: webServer: { command: 'npm run start', url: 'http://localhost:3000', reuseExistingServer: !process.env.CI, }, This option applies to the testConfig.webServer in @playwright/test, not to BrowserServer from BrowserType.launchServer which has no such option.
Citations:
- 1: https://playwright.dev/docs/test-webserver
- 2: https://playwright.dev/docs/test-configuration
- 3: https://playwright.dev/docs/release-notes
Use a dedicated E2E port instead of reusing whatever is on 5173.
With reuseExistingServer: true (enabled locally), Playwright will silently attach to any process already serving http://127.0.0.1:5173. Since that's Vite's default port, a developer running an unrelated Vite project locally will cause tests to run against the wrong server without any warning, leading to false positives or false negatives.
Suggested change
import { defineConfig, devices } from "@playwright/test";
+const baseURL = "http://127.0.0.1:4173";
+
export default defineConfig({
testDir: "./tests",
testMatch: "**/*.spec.ts",
fullyParallel: true,
@@
reporter: "html",
use: {
- baseURL: "http://127.0.0.1:5173",
+ baseURL,
trace: "on-first-retry",
},
@@
webServer: {
- command: "npm run dev",
- url: "http://127.0.0.1:5173",
+ command: "npm run dev -- --port 4173",
+ url: baseURL,
reuseExistingServer: !process.env.CI,
timeout: 60_000,
},
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@desktop/playwright.config.ts` around lines 11 - 25, The Playwright config
currently points at port 5173 (baseURL and webServer.url) which can collide with
any local Vite instance; change to a dedicated E2E port (e.g. 5174) or read from
an env var (e.g. PLAYWRIGHT_E2E_PORT) and update the webServer.command,
webServer.url and use.baseURL to use that port; ensure the webServer.command
passes the port env var to the dev server start command and keep
reuseExistingServer behavior as desired so Playwright no longer silently
attaches to unrelated servers.
Comment on lines
+28
to
+45
| role="tablist" | ||
| data-testid="workspace-tab-pills-scroller" | ||
| className="flex gap-2 overflow-x-auto px-3 py-2 border-b border-white/10 | ||
| [scrollbar-width:none] [-ms-overflow-style:none] | ||
| [&::-webkit-scrollbar]:hidden" | ||
| style={{ | ||
| WebkitOverflowScrolling: "touch", | ||
| }} | ||
| > | ||
| {tabs.map((t) => { | ||
| const isActive = t.id === active; | ||
| return ( | ||
| <button | ||
| key={t.id} | ||
| ref={isActive ? activeRef : undefined} | ||
| role="tab" | ||
| aria-selected={isActive} | ||
| onClick={() => onSelect(t.id)} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
According to the WAI-ARIA Authoring Practices, what attributes and keyboard interactions are required for a tabs pattern using role="tablist", role="tab", and role="tabpanel"?
💡 Result:
According to the WAI-ARIA Authoring Practices Guide (APG) Tabs pattern, the required attributes and keyboard interactions for a tabs implementation using role="tablist", role="tab", and role="tabpanel" are as follows. Required Roles: - Container for tabs: role="tablist" - Individual tabs: role="tab" (children of tablist) - Content panels: role="tabpanel" Required Attributes: For role="tablist": - aria-labelledby or aria-label to provide an accessible name describing the purpose of the tabs. For role="tab": - aria-controls referencing the id of the associated tabpanel. - aria-selected="true" on the active tab; "false" on others. - Use roving tabindex: tabindex="0" on the active/focused tab, tabindex="-1" on inactive tabs (often implemented with buttons, which are naturally focusable). For role="tabpanel": - aria-labelledby referencing the id of the associated tab. - Optionally, tabindex="0" if the panel lacks focusable content. Optional attributes: - aria-orientation="vertical" if vertically oriented (default: horizontal). - aria-haspopup on tabs if they have popup menus. Keyboard Interactions (required for both automatic and manual activation variants): - Tab: Moves focus into the tablist to the active tab; from active tab, moves to next element (often the tabpanel). - Shift+Tab: Moves focus to previous element. - Right Arrow: Moves focus to next tab (wraps to first if on last). - Left Arrow: Moves focus to previous tab (wraps to last if on first). - Home: Moves focus to first tab. - End: Moves focus to last tab. - Space or Enter: Activates the focused tab (shows its panel) in manual activation; automatic activation happens on focus. - Delete (optional): Removes focused tab if supported. Note: APG provides two variants - automatic (activate on focus) and manual (activate on Space/Enter). Manual is recommended unless panels load instantly. Hide inactive tabpanels (e.g., display:none or hidden attribute). Use buttons for tabs to inherit native keyboard support.
Citations:
- 1: https://www.w3.org/WAI/ARIA/apg/patterns/tabs/examples/tabs-automatic/
- 2: https://www.w3.org/WAI/ARIA/apg/patterns/tabs/
- 3: https://www.w3.org/WAI/ARIA/apg/patterns/tabs/examples/tabs-actions/
- 4: https://www.w3.org/WAI/ARIA/apg/patterns/tabpanel/
- 5: https://www.w3.org/TR/wai-aria-practices/examples/tabs/tabs-1/tabs.html
- 6: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/tab_role
- 7: https://www.w3.org/WAI/ARIA/apg/patterns/tabs/examples/tabs-manual/
🏁 Script executed:
cat -n desktop/src/components/mobile/WorkspaceTabPills.tsxRepository: jaylfc/tinyagentos
Length of output: 2153
tablist/tab semantics are incomplete for accessibility.
Lines 28 and 43 declare role="tablist" and role="tab", but the component is missing several required ARIA attributes and behaviors per WAI-ARIA Authoring Practices:
- No
aria-controlslinking tabs to their panels - No roving
tabindex(active tab should betabindex="0", inactivetabindex="-1") - No keyboard handlers for Arrow, Home, End, or Space/Enter keys
- No
aria-labelledbyon the tablist container
This creates confusing screen-reader and keyboard navigation experience. Either implement full tabs pattern semantics (including keyboard handlers and panel associations) or simplify to button/segmented-control roles if panels are managed elsewhere.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@desktop/src/components/mobile/WorkspaceTabPills.tsx` around lines 28 - 45,
The tablist/tab elements lack required ARIA attributes and keyboard behavior;
update the component that renders tabs (the tabs.map callback using activeRef
and onSelect) to either implement the full WAI-ARIA tabs pattern or convert to a
simpler button/segmented-control pattern: if implementing tabs, add
aria-controls on each button pointing to the associated panel id, ensure the
tablist has aria-labelledby, implement roving tabindex (set the active tab
rendered by tabs.map to tabindex="0" and inactive to tabindex="-1"), and add
keyboard handlers on the tab elements to support ArrowLeft/ArrowRight/Home/End
navigation and Space/Enter activation (use onKeyDown alongside onClick and
update focus via activeRef); alternatively, if panels are managed elsewhere,
change roles from role="tablist"/role="tab" to role="group" or leave as plain
buttons and keep existing onSelect.
- Render BoardToolbar above MobileBoardCarousel so the mobile filter sheet is reachable. - Add a Close button to the mobile TaskModal loading state so users aren't trapped if the task fetch resolves null. - Disable backdrop dismissal on TaskCreateSheet while a submit is in flight (matches the Cancel button). - Wire role=tab to role=tabpanel via id/aria-controls/aria-labelledby on both mobile pills and desktop nav. - Add type=button to WorkspaceTabPills tab buttons. - Add aria-label="Task title" to TaskCreateSheet input so assistive tech keeps a stable accessible name once the placeholder is gone. - Make the e2e backend-precondition path SKIP (not silently PASS) when project creation fails.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (10 files)
Reviewed by grok-code-fast-1:optimized:free · 201,519 tokens |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
desktop/tests/projects-mobile.spec.ts (1)
49-50: Consider cleaning up the project/task this spec creates.This test mutates shared backend state and never removes those records, so repeated runs will gradually make the scenario more data-dependent. If the harness has API helpers, deleting the created entities in a
finallyblock would keep the environment stable.Also applies to: 86-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/tests/projects-mobile.spec.ts` around lines 49 - 50, This test creates a persistent project via nameInput.fill(...) and the create button click (page.getByRole("dialog").getByRole("button", { name: /^create$/i }).click()); wrap the create/assert flows in a try/finally and in the finally call your test harness/API helper (e.g., testHarness.deleteProject(projectId) or api.deleteProject(...)) to remove the created project (and any tasks created in the later block around lines 86-88), capturing the created project ID from the response or UI so the cleanup targets the exact entity. Ensure the cleanup is resilient (ignore not-found errors) so repeated runs don’t accumulate state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@desktop/tests/projects-mobile.spec.ts`:
- Around line 49-50: This test creates a persistent project via
nameInput.fill(...) and the create button click
(page.getByRole("dialog").getByRole("button", { name: /^create$/i }).click());
wrap the create/assert flows in a try/finally and in the finally call your test
harness/API helper (e.g., testHarness.deleteProject(projectId) or
api.deleteProject(...)) to remove the created project (and any tasks created in
the later block around lines 86-88), capturing the created project ID from the
response or UI so the cleanup targets the exact entity. Ensure the cleanup is
resilient (ignore not-found errors) so repeated runs don’t accumulate state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f5472d31-b5bf-40ab-ac52-972f554991ad
📒 Files selected for processing (6)
desktop/src/apps/ProjectsApp/ProjectWorkspace.tsxdesktop/src/apps/ProjectsApp/board/ProjectBoard.tsxdesktop/src/apps/ProjectsApp/board/TaskModal.tsxdesktop/src/apps/ProjectsApp/mobile/TaskCreateSheet.tsxdesktop/src/components/mobile/WorkspaceTabPills.tsxdesktop/tests/projects-mobile.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- desktop/src/apps/ProjectsApp/board/ProjectBoard.tsx
- desktop/src/apps/ProjectsApp/board/TaskModal.tsx
- desktop/src/apps/ProjectsApp/ProjectWorkspace.tsx
- desktop/src/components/mobile/WorkspaceTabPills.tsx
This was referenced Apr 30, 2026
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
Phone-targeted UX for ProjectsApp: surgical
useIsMobile()branches plus a smalldesktop/src/apps/ProjectsApp/mobile/subtree. Zero backend changes.MobileSplitViewwraps ProjectList + ProjectWorkspace;WorkspaceTabPillsreplaces the tab strip on phone.ProjectFab+TaskCreateSheetbottom-sheet. Tasks/Members/Activity rows stack via Tailwind responsive classes.MobileBoardCarousel— sticky column pills + swipe-paged columns with section-header swimlanes. No DnD on mobile (status changes via the modal).MobileTaskModal— full-screen, slot-based (Hero/Metadata/SubTasks/Relationships/Activity). Sticky action bar (Claim/Close/Reopen). Swipe-left/right for sibling nav (60px threshold, vertical-dominance + pinch guards). Re-entrancy guard +window.alerton status-change failure.editor.updateInstanceState({ isReadonly })(tldraw v4 API; the prop-basedreadOnlydoesn't exist on this version).viewport-fit=cover, Apple meta tags, manifest, icons, standalone detection all already in place.Notable deviations from the plan
<Tldraw readOnly>prop — used the editor API."Back to Projects", modal closearia-label="Close modal"(disambiguated from the "Close" action button).<section aria-label>(maps toregion) rather than<div role="group">; verified<details>and<dl>carry no implicit role.data-testid="task-card"toTaskListItemso the e2e selector targets task cards, not swimlane toggles.Test plan
projects-mobile.spec.tspasses oniphone-14profile (verified locally)Open follow-ups (non-blocking)
tablist/tablackstabpanelwiring on the workspace tabs (visual UX clear; SR users would benefit).MobileBoardCarouselpanes usew-screenrather thanw-full— fine for full-width windows, will overflow if the projects window is ever embedded narrow.canvas-e2e.spec.tsskeleton needs@backend-requiredtag or deletion (it fails without backend).Summary by CodeRabbit
New Features
Tests
Chores