Project page hub redesign - #905
Conversation
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 78 out of 78 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
apps/desktop/src/main/ipc/tasks-handlers.ts:280
- Similarly, file imports link files to the project via
void domain.linkItemToProject(...), so linking failures won’t be reflected inImportFilesToProjectResult(and can surface as unhandled rejections). This can cause the UI to show “files added” even when the link step fails.
return importFilesToProject(
{
importFiles: async (sourcePaths) => {
const result = await importFiles({ sourcePaths })
return { importedFiles: result.importedFiles, errors: result.errors }
},
getIdByPath: async (destPath) => (await getNoteByPath(destPath))?.id ?? null,
linkToProject: (projectId, fileId) => {
void domain.linkItemToProject({ projectId, itemType: 'file', itemId: fileId })
},
apps/desktop/src/renderer/src/pages/project/use-project-hub.ts:167
- In the error path, the hook logs but never updates
loaded, which leaveshomeNoteIdstuck asundefinedand can make the overview-note rail render the loading spinner indefinitely. Consider setting an explicit fallback payload (empty contents +homeNoteId: null) so the UI can still render and let the user create an overview note even if the IPC call fails.
apps/desktop/src/renderer/src/pages/project/project-capture-input.tsx:100 result.failedcan include entries with an emptypath(e.g. importer-level errors). ToastingfileErrorwith{name: ''}produces a confusing message, and the more actionablefailure.erroris never surfaced to the user.
apps/desktop/src/main/tasks/capture-url.ts:55captureUrlToProjecttreats the page title as link text, but the title is not escaped for markdown. Titles containing](or\) will produce malformed markdown like[...] (url)and can render incorrectly in the created note.
const note = await deps.createNote({
title: title?.trim() || titleFromUrl(input.url),
content: `[${title?.trim() || input.url}](${input.url})\n`
})
apps/desktop/src/renderer/src/pages/project/index.tsx:276
ProjectOverviewNotecallsonHomeNoteChange(noteId)specifically so the parent can updatehomeNoteIdimmediately (avoiding the “create” UI staying visible after create/clear until a refetch completes). HereonHomeNoteChangeignores the argument and only triggershub.refresh(), which can leave a confusing intermediate state.
| return captureUrlToProject( | ||
| { | ||
| fetchTitle: async (url) => (await fetchUrlMetadata(url)).title ?? null, | ||
| createNote: async ({ title, content }) => createNote({ title, content }), | ||
| linkToProject: (projectId, noteId) => { | ||
| void domain.linkItemToProject({ projectId, itemType: 'note', itemId: noteId }) | ||
| } |
The three capture boxes had drifted apart — 12px/18px against 13px/16px text, different placeholder colours, different focus treatment. Geometry now lives in one CaptureBar; capabilities are opt-in props (quickAdd, attachment, voice, onOpenDetail) so each surface keeps its own behaviour without owning its own layout. quick-add-input is superseded and gone. Also groups the hub's rows through hub-groups and folds the project header into the tab bar, so the page keeps one toolbar row instead of two.
Review feedback on #905: - captureUrlToProject and importFilesToProject started the project link and ignored it. A link that failed (deleted project, FK error) still reported success, leaving exactly the orphan note those helpers exist to prevent. Both now await the link and report the failure. - A page title carrying `[` or `]` closed the markdown link early; link text is escaped now. - useProjectHub logged a failed load and stored nothing, so `isLoading` — derived from "no payload for this project" — never cleared and the hub sat on its skeleton for good. It stores an empty payload instead. - Importer-level failures carry no path, so the file toast named a file called "". It shows the underlying error in that case. - The rail's overview note reports its new id; the hub applies it before the refetch instead of leaving the "create" affordance up in between. - rows.test: anchor every alternative in the size-chip regex (CodeQL).
Renumbers the pinned-links migration to 0042: main landed 0040_tag_categories and 0041_tag_definition_views while this branch was open.
| )} | ||
| </div> | ||
|
|
||
| {PROJECT_RAIL_VISIBLE && railOpen ? ( |
| </LayoutGroup> | ||
|
|
||
| <div className="ms-auto flex shrink-0 items-center gap-1"> | ||
| {PROJECT_RAIL_VISIBLE ? ( |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 106 out of 106 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
apps/desktop/src/main/tasks/capture-url.ts:64
- The markdown link destination uses
(${input.url}). URLs containing spaces or parentheses (common in Wikipedia, Google Docs, etc.) will break markdown parsing (similar to the existing note invault/attachments.tsabout parens breaking links). Wrapping the URL in<...>is a simple way to make the destination robust.
const linkText = title?.trim() || input.url
const note = await deps.createNote({
title: title?.trim() || titleFromUrl(input.url),
content: `[${escapeLinkText(linkText)}](${input.url})\n`
})
apps/desktop/src/renderer/src/components/capture-bar/capture-bar.tsx:155
- CaptureBar reads its shortcut/CTA strings from the
commonnamespace (e.g.capture.focusShortcut). Onlyen/common.jsonadds these keys in this PR, while other locales already had localized equivalents underinbox.json. As-is, non-English locales will regress to fallback/English for these capture strings (or show missing-key output depending on i18n config).
const { t } = useT('common')
const [value, setValue] = useState('')
const [isFocused, setIsFocused] = useState(false)
const [isRecording, setIsRecording] = useState(false)
const [isRecorderMounted, setIsRecorderMounted] = useState(false)
const fieldRef = useRef<HTMLTextAreaElement>(null)
const overlayRef = useRef<HTMLDivElement>(null)
const recorderRef = useRef<VoiceRecorderHandle | null>(null)
const recorderDismissTimerRef = useRef<number | null>(null)
// Guards the attach button's pointerdown/click pair (see the button below).
const attachFiredByPointerRef = useRef(false)
const disabled = isBusy
const trimmed = value.trim()
useKeyboardShortcuts([
{
key: 'q',
action: () => fieldRef.current?.focus(),
description: t('capture.focusShortcut')
}
])
apps/desktop/src/main/database/drizzle-data/0042_project_link_pinned.sql:1
- PR description calls the migration
0040_project_link_pinned.sql, but the added migration is0042_project_link_pinned.sql(and the Drizzle journal tag is0042_project_link_pinned). This makes the PR narrative misleading for reviewers and anyone searching for the migration by name/number.
ALTER TABLE `project_links` ADD `pinned` integer DEFAULT 0 NOT NULL;
| * The details rail is built and wired, but we are not showing it to users yet. | ||
| * Hidden behind one flag — the rail, its tab-bar toggle, and the remembered | ||
| * open/closed state all stay in place, so bringing it back is a single edit. | ||
| */ | ||
| export const PROJECT_RAIL_VISIBLE = false |
…-url links Moving the capture bar into `common` left `capture.focusShortcut` English-only while 31 locales still carried a translation under the old inbox key — carry those translations across so non-English users keep the shortcut description. A pasted URL with a space or paren (`…/Yjs_(CRDT)`) ended the markdown destination early; wrap those in angle brackets. The details rail ships behind PROJECT_RAIL_VISIBLE=false, so the E2E now reads the same flag: it asserts the rail and its toggle are absent while the flag is off, and returns to the toggle walkthrough when it is flipped on.
| // proof the rows are there — assert the seeded task is actually visible. | ||
| await expect(page.getByText(taskTitle)).toBeVisible() | ||
|
|
||
| if (PROJECT_RAIL_VISIBLE) { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 137 out of 137 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/i18n/src/locales/en/common.json:6
capture.detailHintis displayed as a user-facing label next to the "⌘/Ctrl ↵" hint in CaptureBar. The current English string "detail" reads like a placeholder; consider using a clearer label (e.g. "Details") to match the rest of the UI copy style.
apps/desktop/src/renderer/src/pages/project/use-relative-time.ts:46formatRelativerounds minutes/hours/days, which can overstate age (e.g. 1h31m → "2h"). Relative-time formatting elsewhere in the renderer usesMath.floor, which avoids jumping early; consider switching to floor here so labels only change after the full unit has elapsed.
A fresh profile starts with the sidebar's Projects section collapsed, and a collapsed section keeps its rows out of the accessibility tree, so the click on the seeded project timed out. Same expand-if-collapsed pattern the tag specs use. Verified: the spec fails on this line without the expand and passes with it (1 passed, 14.0s, local Electron run).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 137 out of 137 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/i18n/src/locales/zh-CN/common.json:644
- This locale adds
common.capture.focusShortcut, butCaptureBaralso readscommon.capture.focusHint,common.capture.detailHint, andcommon.capture.submit. With onlyfocusShortcutpresent, those labels will fall back to another language or render as missing keys in this locale. Add the missingcapture.*keys here (and consistently across all locales) so the shared CaptureBar UI is fully localized.
apps/desktop/src/main/database/queries/projects.ts:653 setProjectLinkPinnedupdates byprojectId+itemIdonly. Sinceproject_links.item_idis shared across multiple item types (note/file/calendar_event), this can accidentally pin/unpin an event link if its id matches. Restrict the update to note/file link rows (or explicitlynote) so pinned state only applies where the hub expects it.
db.update(projectLinks)
.set({ pinned: pinned ? 1 : 0 })
.where(and(eq(projectLinks.projectId, projectId), eq(projectLinks.itemId, itemId)))
.run()
`note-card-pieces.tsx` floors at every step; the hub's copy rounded, so the same note read "2h" in a project row and "1h" on its folder card. The row test's fixture sat exactly on the 2h boundary, which `useNowMinute`'s minute truncation pushes just under — moved it a minute clear.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 137 out of 137 changed files in this pull request and generated no new comments.
Suppressed comments (2)
apps/desktop/src/main/database/queries/projects.ts:657
setProjectLinkPinnedupdates by(projectId, itemId)only and always bumpsprojects.modifiedAt, even if no link row matched. This can accidentally pin non-note links (or multiple rows if the same id exists across item types) and will mark the project as modified even when nothing changed.
db.update(projectLinks)
.set({ pinned: pinned ? 1 : 0 })
.where(and(eq(projectLinks.projectId, projectId), eq(projectLinks.itemId, itemId)))
.run()
apps/desktop/src/main/tasks/capture-url.ts:5
- The header comment claims this avoids leaving a stray note behind if linking fails, but the current implementation can still create the note and then return
{ success: false, noteId, ... }when linking fails (so the stray note still exists). Either update the comment to reflect the actual behavior or add rollback (delete note) on link failure.
/**
* Project hub → "paste a link". Creates a note for the URL and links it to the
* project in one step, so the renderer never has to sequence two mutations and
* leave a stray note behind if the second one fails.
*/
Rewrites the project page as a hub. Everything a project holds — tasks, notes, files, events — is reachable from one page, with per-category focus and navigation that lands you in each item's real home view.
What changed
Header — project name,
12/30 done, and an overdue pill that appears only when something is past due. The icon opens the picker;⋯edits or archives.Capture bar — one input. Plain text becomes a task in this project (the existing quick-add parser still handles
friday p1); a bare URL becomes a note holding that link, already added to the project; the paperclip imports files and links them.Five in-page tabs — Overview / Tasks / Notes / Files / Events. Overview previews five of each category with a
View allthat focuses that tab. None of it opens another app tab: the choice lives intab.viewState, so it survives tab switches.Rows behave like their home view — change a task's status or priority from the row, click a note's icon to change it, click any row to open the item. A linked event opens the Calendar on that event's day with its detail showing, instead of dropping you on today. That reuses
buildRedirectTabfrom the canvas rather than re-deriving the viewState.Details rail — built and wired on all five tabs, collapsible from a toggle that stays in place while closed, so reopening is the same button. Carries the overview note (inline, editable), pinned notes, progress, and details. It ships behind
PROJECT_RAIL_VISIBLE = false— the rail, its toggle, and the remembered open/closed state all stay in place, so turning it on for users is a single edit. Progress renders one row per status the project defines — a project with four in-progress statuses gets four rows — plus an overdue row when relevant.Performance
The old page called
tasks:project-list-linksfour times per load (once by the page, once inside each of the three section components) and then resolved every link with its own IPC round trip: ~74 calls for a project with 70 links. One newtasks:project-list-contentsjoinsproject_linksagainstnote_metadataandcalendar_eventsin a single pass. The join also drops orphaned links, so the defensive null-filtering each section did is gone.Schema and sync
One additive migration,
0042_project_link_pinned.sql:project_links.pinned INTEGER NOT NULL DEFAULT 0, for the notes pinned to the overview. Every existing link keeps its current behaviour.Backward-compat landmine, handled: links reconcile wholesale rather than field-merging, and the update branch wrote a fixed column set. A payload pushed by a client that predates this change carries no
pinnedkey, so it would have wiped every local pin.reconcileLinksnow falls back to the existing row's value, with a test that fails if the fallback is removed (verified by mutating it).No new
SyncItemType, no server change. "Updated" in the details panel needed no column at all —projects.modified_atalready exists, is stamped by every mutation, and is already field-merged.Removed
project-home.tsx,project-stats-row, and theproject-{notes,files,events}-sectioncomponents are superseded.projects-tab-content.tsxwent with them — it was already unreachable from the app, with only a test importing it.project-selector.tsxstays;project-pickeranduse-project-quick-createstill use it.Verification
lint(0 errors, 0 new warnings),typecheck,test,ipc:check,check:architecture,check:contracts,i18n:check,docs:impact --strict,docs:build— all green. 49 unit tests cover the new page.The E2E spec has not run.
tests/e2e/project-hub.e2e.tsis new and untested locally: Electron fails to launch on this machine during fixture setup, and a pre-existing spec (tasks-kanban) fails identically at the same point, so this is the known local-launch problem rather than the new spec. CI is its first real run.Known limitation
The Events section's
+opens the Calendar with a create intent; the new event is not linked to the project automatically. Linking still goes through the existing calendar chip menu. Adding a project-side event picker was out of scope here.🤖 Generated with Claude Code