feat: artifact and project management UI in web dashboard#954
feat: artifact and project management UI in web dashboard#954
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ 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). (7)
🧰 Additional context used📓 Path-based instructions (1)web/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (web/CLAUDE.md)
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2026-03-31T14:41:06.345ZApplied to files:
🔇 Additional comments (2)
WalkthroughAdds backend WebSocket channels and event types for artifacts and projects and a request-scoped helper to publish WS events. Extends Artifact and CreateArtifactRequest models with project association, content type, and size. Controllers publish artifact/project WS events on create/delete/upload flows. Frontend: registers Projects and Artifacts routes and pages; adds many UI components (MetadataGrid, ProjectStatusBadge, ContentTypeBadge, cards, filters, skeletons), hooks (including reusable useDetailData, useArtifactsData, useProjectsData, detail hooks), Zustand stores, API endpoints, MSW mocks, file utilities (formatFileSize, downloadArtifactFile), and comprehensive tests. Sidebar, router routes, and shared API/types updated. Suggested labels
🚥 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. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches. Re-running this action after a short time may resolve the issue. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
There was a problem hiding this comment.
Code Review
This pull request introduces the Projects and Artifacts dashboard modules, encompassing backend WebSocket event triggers, frontend API services, Zustand state management, and a suite of new UI components and pages. The changes include comprehensive unit tests and design documentation updates. Review feedback identified critical Python 3 syntax errors in exception handling within the controllers and suggested improvements for the file size formatting utility and artifact content preview logic to ensure robust error handling and accurate syntax highlighting.
| event.model_dump_json(), | ||
| channels=[CHANNEL_ARTIFACTS], | ||
| ) | ||
| except MemoryError, RecursionError: |
There was a problem hiding this comment.
This line uses Python 2 syntax for catching multiple exceptions, which results in a SyntaxError in Python 3. Multiple exceptions must be enclosed in a tuple.
| except MemoryError, RecursionError: | |
| except (MemoryError, RecursionError): |
| event.model_dump_json(), | ||
| channels=[CHANNEL_PROJECTS], | ||
| ) | ||
| except MemoryError, RecursionError: |
There was a problem hiding this comment.
This line uses Python 2 syntax for catching multiple exceptions, which results in a SyntaxError in Python 3. Multiple exceptions must be enclosed in a tuple.
| except MemoryError, RecursionError: | |
| except (MemoryError, RecursionError): |
web/src/utils/format.ts
Outdated
| if (!Number.isFinite(bytes) || bytes < 0) return '--' | ||
| if (bytes === 0) return '0 B' | ||
| const units = ['B', 'KB', 'MB', 'GB', 'TB'] | ||
| const exponent = Math.min(Math.floor(Math.log(bytes) / Math.log(1024)), units.length - 1) |
There was a problem hiding this comment.
The exponent calculation can result in -1 if bytes is between 0 and 1 (e.g., 0.5), which would cause an out-of-bounds access on the units array. Clamping the exponent to a minimum of 0 ensures robustness for small fractional values.
| const exponent = Math.min(Math.floor(Math.log(bytes) / Math.log(1024)), units.length - 1) | |
| const exponent = Math.max(0, Math.min(Math.floor(Math.log(bytes) / Math.log(1024)), units.length - 1)) |
| function getLanguage(contentType: string): 'json' | 'yaml' { | ||
| if (contentType === 'application/json') return 'json' | ||
| if (contentType === 'application/x-yaml' || contentType === 'text/yaml') return 'yaml' | ||
| // Default to JSON for syntax highlighting even for plain text | ||
| return 'json' | ||
| } |
There was a problem hiding this comment.
Defaulting to json syntax highlighting for all non-YAML text files (like markdown or plain text) will cause the editor to display numerous syntax errors. It is better to return undefined for unsupported types to disable highlighting or add support for more languages like markdown.
| function getLanguage(contentType: string): 'json' | 'yaml' { | |
| if (contentType === 'application/json') return 'json' | |
| if (contentType === 'application/x-yaml' || contentType === 'text/yaml') return 'yaml' | |
| // Default to JSON for syntax highlighting even for plain text | |
| return 'json' | |
| } | |
| function getLanguage(contentType: string): string | undefined { | |
| const lower = contentType.toLowerCase() | |
| if (lower === 'application/json') return 'json' | |
| if (lower === 'application/x-yaml' || lower === 'text/yaml') return 'yaml' | |
| if (lower === 'text/markdown') return 'markdown' | |
| return undefined | |
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #954 +/- ##
==========================================
- Coverage 92.02% 92.00% -0.02%
==========================================
Files 612 612
Lines 32984 33021 +37
Branches 3169 3170 +1
==========================================
+ Hits 30352 30380 +28
- Misses 2089 2097 +8
- Partials 543 544 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
64a65c3 to
87d5b82
Compare
There was a problem hiding this comment.
Actionable comments posted: 30
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/api/test_channels.py`:
- Around line 45-46: The test is reaching into internals
(plugin._arbitrary_channels_allowed and plugin._channels) which is fragile;
either assert the same behavior via a public API if available (e.g.,
plugin.is_channel_allowed(...) or plugin.allowed_channels property) or, if no
public accessor exists, keep the assertions but add a brief comment above the
asserts referencing why accessing _arbitrary_channels_allowed and _channels is
necessary for security verification and that they are intentionally relied upon
(include ALL_CHANNELS as the expected constant); update tests to prefer a public
method first and only fall back to the private attributes with the explanatory
comment.
In `@web/src/__tests__/pages/ArtifactDetailPage.test.tsx`:
- Around line 49-82: Add property-based tests using fast-check to cover state
transitions for ArtifactDetailPage by replacing or augmenting the example-based
tests that mutate hookReturn; use fc.assert and fc.property to generate
combinations of relevant state fields (e.g., artifact present vs null, loading
true/false, error string vs null) and call renderPage for each generated state,
then assert UI invariants like presence of 'artifact-detail-skeleton' when
loading & artifact null, 'Artifact not found.' when artifact null & not loading,
presence of 'artifact-metadata' and 'artifact-content-preview' when artifact is
set, and error banner text when error is set; reference the existing
helpers/variables hookReturn, defaultHookReturn, and renderPage to build the
generated states and reuse testIds/text checks (artifact-detail-skeleton,
artifact-metadata, artifact-content-preview, 'Artifact not found.', and error
string).
In `@web/src/__tests__/pages/ArtifactsPage.test.tsx`:
- Around line 46-86: Add property-based tests using fast-check in
web/src/__tests__/pages/ArtifactsPage.test.tsx to cover core invariants: use
fc.assert + fc.property to generate arbitrary valid permutations of the hook
state (use existing defaultHookReturn as a base and vary fields like loading,
error (string | undefined), totalArtifacts, artifacts, filteredArtifacts,
wsConnected) and call the existing renderPage helper for each generated state to
assert invariants such as "page heading 'Artifacts' is always present", "no
crash when error is any string and alert shows text when error defined", and
"skeleton is only shown when loading && totalArtifacts === 0". Reuse hookReturn
and renderPage names to inject the generated states and keep each property test
focused on one invariant (stable render, error-handling, loading transition).
In `@web/src/__tests__/pages/ProjectDetailPage.test.tsx`:
- Around line 52-86: Add a property-based test using fast-check that asserts
ProjectDetailPage behavior across generated combinations of the hook state (use
fc.assert(fc.property(...))). Generate booleans/nullable values for the relevant
pieces from this test file (loading, project nullable, and error string) and for
each generated state set hookReturn accordingly (referencing defaultHookReturn
and hookReturn) then call renderPage and assert the same invariants already
covered (skeleton when loading && project null, "Project not found." when
!loading && project null, error banner when error set, and presence of
header/team/task when project present). Ensure the new test imports fc and uses
fc.assert(fc.property(...)) so it follows the repository guideline for
web/src/**/*.test.{ts,tsx}.
In `@web/src/__tests__/stores/artifacts.test.ts`:
- Around line 104-112: The test currently calls
useArtifactsStore.getState().updateFromWsEvent({} as never) which hides the
expected event shape; replace the synthetic never payload with a realistic event
object that matches the handler's expected structure (e.g., include properties
like type/action/resource/id/timestamp or whatever updateFromWsEvent reads) so
the test documents the contract and will fail if the handler begins relying on
payload fields; keep listArtifacts mocked as before and call updateFromWsEvent
with that realistic event to ensure listArtifacts is still invoked.
- Around line 57-77: Add a test to verify partial-failure behavior in
fetchArtifactDetail: mock getArtifact to resolve with an artifact (use
makeArtifact) and mock getArtifactContentText to reject (e.g., new
Error('Content unavailable')), then call
useArtifactsStore.getState().fetchArtifactDetail('artifact-001') and assert that
useArtifactsStore.getState().selectedArtifact equals the artifact while
contentPreview is cleared/nullable (e.g., null) and detailError is not set to
the content error (or matches the expected fallback behavior); target the
fetchArtifactDetail flow and the mocks for getArtifact and
getArtifactContentText when adding this case.
- Around line 114-124: Replace or augment the existing unit tests for the filter
setters with property-based tests using fast-check: write
fc.assert(fc.property(fc.string(), (s) => {
useArtifactsStore.getState().setSearchQuery(s); return
useArtifactsStore.getState().searchQuery === s })) and similarly for
setTypeFilter (useArtifactsStore.getState().setTypeFilter) to verify arbitrary
strings (including empty, unicode, long) round-trip correctly; use fc.assert +
fc.property and import fast-check as fc, keeping the test descriptions around
"sets search query" and "sets type filter".
In `@web/src/__tests__/stores/projects.test.ts`:
- Around line 120-128: The test calls
useProjectsStore.getState().updateFromWsEvent which fires fetchProjects()
asynchronously, so the assertion on mocked listProjects may race; update the
test to wait for the async call to complete (e.g., use waitFor from
`@testing-library/react` or a small await flush-promises/nextTick) before
asserting listProjects was called. Locate the test around the updateFromWsEvent
spec and change the expect(listProjects).toHaveBeenCalled() to run inside a
waitFor or after awaiting a promise resolution to ensure fetchProjects has
executed.
In `@web/src/components/ui/metadata-grid.stories.tsx`:
- Around line 13-57: Add three exported Story objects—Hover, Loading, and
Error—next to the existing Default, TwoColumns, FourColumns, WithMonoValues, and
Empty exports; each should provide args that represent the required UI states
(e.g., Hover: include a representative items array and a hover/hovered boolean
arg to simulate the hover state; Loading: set isLoading: true and optionally no
items; Error: set an error or errorMessage string arg and optionally items
empty) and ensure the arg names match the actual MetadataGrid component props
(adjust hover/isLoading/error keys to the component's real prop names).
In `@web/src/components/ui/metadata-grid.tsx`:
- Around line 24-41: The component currently renders label/value pairs with
generic div/span markup which loses semantic relationship for screen readers;
change the outer container to a <dl> instead of the wrapping div, render each
pair as a wrapper element (e.g., a div) containing a <dt> for item.label and a
<dd> for item.value, preserve the existing styling by applying the same
className, colClass and per-item item.valueClassName to the corresponding
elements (keep the key on the pair using item.label), and ensure the
small-uppercase label classes are moved to the <dt> and the 'text-foreground'
plus item.valueClassName are applied to the <dd> so visual styling remains
identical while gaining proper metadata semantics.
In `@web/src/components/ui/project-status-badge.tsx`:
- Around line 34-40: Replace the inline status dot and label with the shared
StatusBadge component: remove the <span data-slot="status-dot" ... /> and the
conditional label span and instead render <StatusBadge> (pass the current status
prop and the label or showLabel flag as props so it displays the dot and label
consistently). Ensure any usage of DOT_COLOR_CLASSES and the local "status-dot"
slot is removed and that the component uses the shared StatusBadge API so
styling and behavior remain centralized.
In `@web/src/hooks/useArtifactDetailData.ts`:
- Around line 1-98: Both useArtifactDetailData and useProjectDetailData
duplicate the same polling, websocket debounce, bindings creation, and lifecycle
logic; extract a reusable hook (e.g., useDetailData or makeUseDetailData
factory) that accepts unique pieces (id, selectors/getters like
useArtifactsStore.getState().fetchArtifactDetail or
useProjectsStore.getState().fetchProjectDetail, store selectors for selected
item/content/loading/error, and DETAIL_CHANNELS) and internally re-uses
usePolling, useWebSocket, WS_DEBOUNCE_MS, DETAIL_POLL_INTERVAL,
artifactIdRef/pollFn/bindings creation and cleanup; then simplify
useArtifactDetailData and useProjectDetailData to call the generic hook with
their specific fetch function, selectors, and channel list.
In `@web/src/hooks/useProjectDetailData.ts`:
- Line 30: The parameter for useProjectDetailData is declared as projectId:
string but the function treats it as optional; update the signature to make
optionality explicit (e.g., change to projectId?: string or projectId: string |
null) and update any internal null/undefined checks to match (keep the existing
if (!projectId) guard). Also update call sites to pass possibly undefined/null
values or narrow before calling, and adjust any related types (e.g.,
UseProjectDetailDataReturn usages) if they rely on a non-optional projectId.
In `@web/src/mocks/handlers/artifacts.ts`:
- Around line 61-67: The GET artifact handler returns apiSuccess(null) with a
404 status which contradicts the HTTP status; update the
http.get('/api/v1/artifacts/:id') handler so when mockArtifacts.find(...) yields
no artifact it returns the project's error envelope (e.g., apiError or the
existing error helper) containing a clear "not found" message instead of
apiSuccess(null), still using HttpResponse.json(..., { status: 404 }); modify
the handler around the artifact lookup (mockArtifacts, params.id) to use that
error helper so the body’s success flag matches the 404 status.
In `@web/src/pages/artifacts/ArtifactCard.tsx`:
- Around line 15-37: ArtifactCard is building a custom "card-with-header" UI but
the codebase requires using the shared SectionCard primitive; replace the outer
Link/div composition in the ArtifactCard component with SectionCard (importing
SectionCard at top), move the artifact path into SectionCard's header area (or
header prop) and place the current body content (StatPill, ContentTypeBadge,
size/created at, and "by {artifact.created_by}") into SectionCard's children,
keeping the Link navigation (either make the SectionCard clickable or wrap the
internal content in the existing Link) and preserve use of formatLabel,
ContentTypeBadge, StatPill, formatFileSize and formatRelativeTime; remove the
handmade card CSS classes and ensure imports are updated (remove unused styles)
so the component relies on SectionCard for layout and styling.
In `@web/src/pages/artifacts/ArtifactContentPreview.tsx`:
- Around line 17-24: getLanguage currently maps only JSON/YAML and returns
'json' for everything else which mislabels types like text/markdown or
text/html; update the getLanguage(contentType: string) function to add mappings
for common types (e.g., 'text/markdown' -> 'markdown' or 'markdown' mode name
used by your editor, 'text/html' -> 'html', 'text/css' -> 'css', etc.) and use a
neutral fallback such as 'text' (or CodeMirror's equivalent) instead of
defaulting to 'json' so non-JSON content doesn't get JSON highlighting; ensure
you reference the same function name getLanguage and the contentType parameter
when locating where to add the extra content-type checks.
- Around line 75-77: The handler handleDownload is declared after the
component's early return so it's inaccessible in the "no content" branch and is
re-created on every render; move the function definition above the early returns
or wrap it in useCallback and define it before any return so it can be
referenced from both branches without being recreated each render — implement
useCallback(() => downloadArtifactFile(artifact.id,
artifact.path.split('/').pop() ?? artifact.id), [artifact.id, artifact.path])
(or equivalent) and replace the current handleDownload reference with this
memoized function.
In `@web/src/pages/artifacts/ArtifactFilters.tsx`:
- Around line 29-95: Replace the raw HTML inputs/selects in ArtifactFilters.tsx
with the dashboard's shared primitives InputField and SelectField (or add
compact variants there) so styling and behavior are centralized: for the search
box use InputField bound to searchQuery and setSearchQuery; for type/content
selects use SelectField bound to typeFilter/setTypeFilter and
contentTypeFilter/setContentTypeFilter and render options from
ARTIFACT_TYPE_VALUES and CONTENT_TYPE_OPTIONS (keep formatLabel for labels); for
createdBy/task/project filters use InputField bound to
createdByFilter/setCreatedByFilter, taskIdFilter/setTaskIdFilter, and
projectIdFilter/setProjectIdFilter. Ensure prop mapping matches the primitives'
API (value, onChange handlers that call the existing setters, placeholder,
aria-label) and remove the duplicated className styling so the shared components
control appearance.
In `@web/src/pages/artifacts/ArtifactGridView.tsx`:
- Line 23: The class on StaggerGroup uses hardcoded pixel breakpoints
(max-[1279px], max-[767px]); replace them with standard Tailwind breakpoint
tokens and a mobile-first pattern (e.g., set base grid-cols-1 then
sm:grid-cols-2 and lg:grid-cols-3 or whichever mapping fits the design) so
layout uses sm/md/lg/xl tokens instead of pixel utilities; update the className
on StaggerGroup to remove max-[1279px]:grid-cols-2 and max-[767px]:grid-cols-1
and use the appropriate tokenized classes (for example: "grid grid-cols-1
sm:grid-cols-2 lg:grid-cols-3 gap-grid-gap") to comply with the tokenized layout
rules.
In `@web/src/pages/projects/ProjectCard.tsx`:
- Around line 15-45: The file uses a bespoke Link-wrapped card shell in
ProjectCard; replace that with the shared SectionCard component so the layout
uses the canonical card-with-header pattern. Swap the outer Link block for
SectionCard (use SectionCard's prop that renders as a Link or wraps navigation),
move the project title + ProjectStatusBadge into the SectionCard header, put
project.description and the StatPill row into the card body, and move the
members/deadline row into the card footer; keep existing child components
(ProjectStatusBadge, StatPill, formatCurrency, formatRelativeTime) and the
routes/encoding used for navigation so behavior remains identical. Ensure
accessibility and existing classes/spacing are preserved via SectionCard props
rather than rebuilding the card markup manually.
In `@web/src/pages/projects/ProjectCreateDrawer.tsx`:
- Around line 100-108: The visible "Team Members" label is not associated with
the TagInput, causing loss of accessibility; remove the standalone <label> and
instead pass the label/aria-label prop into the TagInput (or provide an id prop
and ensure TagInput attaches it to its underlying input) so that TagInput
(component TagInput) receives the accessible label for form.team updates via
updateField; update the JSX to stop rendering the sibling label and supply the
text "Team Members" to TagInput (or set aria-labelledby/aria-label inside
TagInput) so assistive tech announces the field on focus.
- Around line 45-63: The budget validation in handleSubmit currently allows
non-finite values (e.g., Infinity from Number('1e309')) and the error text says
"positive" while 0 is accepted; update the validation to parse the budget into a
numeric value (const parsed = Number(form.budget)), require
Number.isFinite(parsed) and parsed >= 0, and set next.budget to a clearer
message like "Budget must be a non-negative finite number" when it fails; only
pass Number(form.budget) into createProject after this check so Infinity/NaN
never get serialized, and keep references to handleSubmit, form.budget,
setErrors, and useProjectsStore.getState().createProject to locate changes.
In `@web/src/pages/projects/ProjectFilters.tsx`:
- Around line 13-41: Replace the raw <input> and <select> with the shared
InputField and SelectField primitives: use InputField for the search box wired
to searchQuery and setSearchQuery (preserve placeholder and aria-label), and use
SelectField for the status dropdown wired to statusFilter and setStatusFilter
(preserve empty "All statuses" option and aria-label). For SelectField, provide
options derived from PROJECT_STATUS_VALUES mapped to { value: s, label:
formatLabel(s) } plus an empty value for clearing; ensure the onChange handler
converts empty string to null and validates values against
PROJECT_STATUS_VALUES/ProjectStatus before calling setStatusFilter. Keep
existing className/styling props and accessibility attributes when replacing the
components.
In `@web/src/pages/projects/ProjectGridView.tsx`:
- Line 23: The StaggerGroup element's className uses hardcoded pixel breakpoints
(max-[1279px], max-[767px]) — update the responsive classes to use standard
Tailwind breakpoint tokens (e.g., xl:, lg:, md:, sm:) so the grid uses
grid-cols-3 at large screens and falls back to grid-cols-2 / grid-cols-1 at the
recommended breakpoints; locate the StaggerGroup in ProjectGridView.tsx and
replace the custom max-[...] tokens with the appropriate Tailwind tokens to
match the intended breakpoints while preserving the grid and gap classes.
In `@web/src/pages/projects/ProjectTaskList.tsx`:
- Around line 29-41: The mapped JSX for each task is too large inline; extract
it into a new Presentational component named ProjectTaskRow that accepts a task
prop (type matching the task shape). Move the Link, TaskStatusIndicator, title
span, and the conditional assigned_to span into ProjectTaskRow and keep the same
className, ROUTES.TASK_DETAIL.replace(':taskId', encodeURIComponent(task.id))
usage and key semantics (ensure key remains on the mapped element or on the
returned <ProjectTaskRow /> in the map). Replace tasks.map((task) => ( ... ))
with tasks.map((task) => <ProjectTaskRow key={task.id} task={task} />) so
behavior and conditional rendering are preserved.
In `@web/src/pages/projects/ProjectTeamSection.tsx`:
- Around line 29-43: Extract the JSX inside project.team.map in
ProjectTeamSection.tsx into a new shared component (e.g., TeamMemberRow) to
comply with the rule against complex JSX in map callbacks: create a
TeamMemberRow component that accepts props (agentId: string, isLead: boolean)
and renders the Link using ROUTES.AGENT_DETAIL, Avatar, the agentId span, and
the conditional "Lead" badge based on isLead; then replace the inline map with
project.team.map(agentId => <TeamMemberRow key={agentId} agentId={agentId}
isLead={agentId === project.lead} />).
In `@web/src/stores/artifacts.ts`:
- Around line 1-9: Update the isPreviewableText function to treat YAML as
previewable by including its MIME type(s) — add checks for "application/yaml"
(and optionally "application/x-yaml") alongside the existing "text/*" and
"application/json" checks so YAML content can render inline previews (modify the
isPreviewableText helper).
- Around line 76-106: The artifact preview check in fetchArtifactDetail relies
on the truthiness of artifact.size_bytes; make it explicit by guarding against
null/undefined before comparing (e.g., check artifact.size_bytes != null &&
artifact.size_bytes > 0) so isPreviewableText and getArtifactContentText only
run when size_bytes is a valid number; update the condition that currently reads
artifact.content_type && artifact.size_bytes > 0 to explicitly test for
non-null/undefined size_bytes and then > 0 to improve readability and
correctness.
In `@web/src/stores/projects.ts`:
- Around line 63-96: The outer try/catch around Promise.allSettled in
fetchProjectDetail is unnecessary because Promise.allSettled never rejects;
remove the try/catch and simplify the flow to rely on projectResult.status and
tasksResult.status for error handling: keep the Promise.allSettled([...]) call,
check projectResult and tasksResult as you already do, ensure you call set({
detailLoading: false, detailError: ... }) on all early returns (when project is
null) and at the end when setting selectedProject/projectTasks, and use
getErrorMessage(projectResult.reason || tasksResult.reason) where appropriate;
reference fetchProjectDetail, projectResult, tasksResult, getErrorMessage and
the existing set(...) calls when making these edits.
In `@web/src/utils/download.ts`:
- Around line 11-20: The current flow creates an object URL with
URL.createObjectURL(blob) and a temporary anchor element (created via
document.createElement('a')) but only cleans them up on the happy path; wrap the
DOM append/click sequence in a try/finally so cleanup always runs: after
obtaining blob from downloadArtifactContent(artifactId) create url and element,
then in a try block append the anchor and call a.click(), and in the finally
block remove the anchor from document.body if it was appended and call
URL.revokeObjectURL(url); reference the downloadArtifactContent,
URL.createObjectURL, URL.revokeObjectURL and the temporary anchor element to
locate where to add the finally cleanup.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 75fa963a-eaca-47dc-8bdc-5f4ce93d8cfa
📒 Files selected for processing (62)
docs/design/brand-and-ux.mddocs/design/page-structure.mdsrc/synthorg/api/channels.pysrc/synthorg/api/controllers/artifacts.pysrc/synthorg/api/controllers/projects.pysrc/synthorg/api/dto.pysrc/synthorg/api/ws_models.pysrc/synthorg/core/artifact.pysrc/synthorg/core/project.pytests/unit/api/test_channels.pyweb/CLAUDE.mdweb/src/__tests__/helpers/factories.tsweb/src/__tests__/pages/ArtifactDetailPage.test.tsxweb/src/__tests__/pages/ArtifactsPage.test.tsxweb/src/__tests__/pages/ProjectDetailPage.test.tsxweb/src/__tests__/pages/ProjectsPage.test.tsxweb/src/__tests__/stores/artifacts.test.tsweb/src/__tests__/stores/projects.test.tsweb/src/api/endpoints/artifacts.tsweb/src/api/endpoints/projects.tsweb/src/api/types.tsweb/src/components/layout/Sidebar.tsxweb/src/components/ui/content-type-badge.stories.tsxweb/src/components/ui/content-type-badge.tsxweb/src/components/ui/metadata-grid.stories.tsxweb/src/components/ui/metadata-grid.tsxweb/src/components/ui/project-status-badge.stories.tsxweb/src/components/ui/project-status-badge.tsxweb/src/hooks/useArtifactDetailData.tsweb/src/hooks/useArtifactsData.tsweb/src/hooks/useProjectDetailData.tsweb/src/hooks/useProjectsData.tsweb/src/mocks/handlers/artifacts.tsweb/src/mocks/handlers/index.tsweb/src/mocks/handlers/projects.tsweb/src/pages/ArtifactDetailPage.tsxweb/src/pages/ArtifactsPage.tsxweb/src/pages/ProjectDetailPage.tsxweb/src/pages/ProjectsPage.tsxweb/src/pages/artifacts/ArtifactCard.tsxweb/src/pages/artifacts/ArtifactContentPreview.tsxweb/src/pages/artifacts/ArtifactDetailSkeleton.tsxweb/src/pages/artifacts/ArtifactFilters.tsxweb/src/pages/artifacts/ArtifactGridView.tsxweb/src/pages/artifacts/ArtifactMetadata.tsxweb/src/pages/artifacts/ArtifactsSkeleton.tsxweb/src/pages/projects/ProjectCard.tsxweb/src/pages/projects/ProjectCreateDrawer.stories.tsxweb/src/pages/projects/ProjectCreateDrawer.tsxweb/src/pages/projects/ProjectDetailSkeleton.tsxweb/src/pages/projects/ProjectFilters.tsxweb/src/pages/projects/ProjectGridView.tsxweb/src/pages/projects/ProjectHeader.tsxweb/src/pages/projects/ProjectTaskList.tsxweb/src/pages/projects/ProjectTeamSection.tsxweb/src/pages/projects/ProjectsSkeleton.tsxweb/src/router/index.tsxweb/src/router/routes.tsweb/src/stores/artifacts.tsweb/src/stores/projects.tsweb/src/utils/download.tsweb/src/utils/format.ts
Add full browse/view/manage UI for artifacts and projects: Backend: WS channels (artifacts, projects), 5 event types, publish on create/delete/upload mutations in artifact and project controllers. Frontend: typed API endpoints, Zustand stores with polling + WebSocket bindings, data hooks, route constants, sidebar navigation. New shared components: MetadataGrid (detail page metadata display), ProjectStatusBadge (semantic status colors), ContentTypeBadge (MIME type display) -- all with Storybook stories. Artifacts: list page with search/type filter, detail page with metadata grid, inline content preview (text via CodeMirror, images via blob URL), download, and delete with confirmation. Projects: list page with search/status filter and create button, create drawer (6 fields including TagInput for team), detail page with header, team section (Avatar grid with lead badge), and linked task list. Tests: store tests, page render tests, MSW handlers, test factories. Closes #946 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move Request import from TYPE_CHECKING guard to runtime import in artifact and project controllers. PEP 649 lazy annotations still require runtime availability for Litestar's dependency injection to resolve handler parameters. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Pre-reviewed by 8 agents, 17 findings addressed: - Add exc_info=True to WS publish error logging (artifacts, projects) - Fix blob URL memory leak in ArtifactContentPreview (ref-based cleanup) - Surface image preview errors to user instead of silent catch - Add partial failure reporting for content preview fetch in store - Add agent/task filter inputs to ArtifactFilters UI - Add INFO log after project creation (PERSISTENCE_PROJECT_SAVED) - Optimistically add project to list on create for immediate UI update - Fix Artifact.created_at nullability (string | null) to match backend - Fix CreateProjectRequest.lead nullability (string | null) to match backend - Fix hardcoded p-3 to p-card in ProjectCreateDrawer error banner - Update page-structure.md: 9 WS channels, remove deferred references, add Projects/Artifacts to nav hierarchy, URL routing, WS subscription map - Update web/CLAUDE.md: add 3 new components to reuse table, update hooks/stores descriptions - Update brand-and-ux.md: add components, formatFileSize, types Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Security: remove image/svg+xml from safe content types (XSS risk), exclude SVG from image preview rendering - Backend: add project_id field to Artifact model and CreateArtifactRequest, extract shared publish_ws_event helper into channels.py, extract _save_metadata_with_rollback helper for upload_content, fix ProjectStatusFilter to use NotBlankStr, fix lead null coercion in WS payload, add Final annotation to ALL_CHANNELS - Frontend: add content type and project filters to artifacts list page, extract shared downloadArtifactFile utility, add image loading skeleton, include error context in partial failure messages, remove fire-and-forget background refresh in createProject - Types: fix WsEvent.payload to dict[str, object], add reserved comment to PROJECT_STATUS_CHANGED - Docs: add Projects and Artifacts page description sections to page-structure.md, fix controller count 20->22, fix budget docstring USD->EUR, add missing Artifact docstring attributes - Tests: strengthen test_create_channels_plugin assertions, add deleteArtifact error propagation test, add updateFromWsEvent tests, add createProject optimistic state verification - Comments: fix CRUD docstrings to match actual operations, add ESLint disable justifications Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Semantics: MetadataGrid uses dl/dt/dd instead of generic div/span
- Accessibility: ProjectCreateDrawer team field uses role="group" with
aria-label, budget validation rejects Infinity/NaN
- Tests: add fast-check property-based tests to ArtifactDetailPage,
ArtifactsPage, and ProjectDetailPage; add partial-failure test and
property-based filter tests to artifacts store; fix updateFromWsEvent
race in projects store with waitFor; use realistic WsEvent payloads
instead of {} as never
- Hooks: extract shared useDetailData factory hook to deduplicate
useArtifactDetailData and useProjectDetailData; make projectId
parameter optional (string | undefined)
- Components: extract ProjectTaskRow and TeamMemberRow from map callbacks
in ProjectTaskList and ProjectTeamSection
- Layout: replace hardcoded pixel breakpoints (max-[1279px], max-[767px])
with standard Tailwind tokens (sm:grid-cols-2 xl:grid-cols-3) in both
grid views
- Stores: add YAML to isPreviewableText, explicit size_bytes null check,
remove unnecessary try/catch around Promise.allSettled in projects store
- Fixes: download utility uses try/finally for DOM cleanup, MSW artifact
404 handler uses apiError instead of apiSuccess(null),
handleDownload moved before early returns with useCallback,
test_channels private attr access documented with comment
- Stories: add SingleItem and ManyItems stories to MetadataGrid
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
87d5b82 to
5440f22
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
web/src/components/ui/metadata-grid.stories.tsx (1)
13-80: 🧹 Nitpick | 🔵 TrivialAdd missing
Hover,Loading, andErrorstory states.The story file covers Default, variants, and Empty, but is missing three required states per coding guidelines. Even for purely presentational components, these stories help document all possible visual states.
🧩 Example additions
export const Empty: Story = { args: { items: [] }, } + +export const Hover: Story = { + parameters: { pseudo: { hover: true } }, + args: Default.args, +} + +export const Loading: Story = { + args: { + items: [ + { label: 'Type', value: '—' }, + { label: 'Status', value: '—' }, + ], + }, +} + +export const Error: Story = { + args: { + items: [{ label: 'Error', value: 'Failed to load metadata' }], + }, +}As per coding guidelines, "Storybook stories (.stories.tsx files) must include all component states: default, hover, loading, error, empty."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/ui/metadata-grid.stories.tsx` around lines 13 - 80, Add three stories named Hover, Loading, and Error alongside the existing exports (Default, TwoColumns, FourColumns, WithMonoValues, SingleItem, ManyItems, Empty). For Loading export a Story that sets a loading prop (e.g., args: { isLoading: true } or items: undefined) to show the spinner/placeholder state; for Error export a Story with an error prop (e.g., args: { error: 'Failed to load' }) to render the component's error UI; for Hover export a Story that provides normal args (reuse Default.items) and a play function that simulates a hover over the first metadata item (use testing-library userEvent.hover on the component's root or first label) so the hover visual state is captured. Ensure the new exports are named exactly Hover, Loading, and Error to match storybook expectations.web/src/pages/projects/ProjectCreateDrawer.tsx (1)
103-111:⚠️ Potential issue | 🟠 MajorAssociate “Team Members” with the actual TagInput control, not only the group.
The visible text is not programmatically bound to the input rendered by
TagInputin this usage. Please pass an explicit input label (label/aria-label/aria-labelledby, depending onTagInputAPI) so assistive tech announces the field name on focus.#!/bin/bash # Verify TagInput supports explicit labeling props and how they map to the underlying input. fd -i 'tag-input.tsx' web/src/components/ui --exec cat rg -n -C3 'interface .*TagInput|type .*TagInput|label|aria-label|aria-labelledby|htmlFor|id=' web/src/components/ui/tag-input.tsx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/projects/ProjectCreateDrawer.tsx` around lines 103 - 111, The visible "Team Members" text is not programmatically bound to the TagInput control, so assistive tech won't announce it; update the TagInput usage in ProjectCreateDrawer (the TagInput rendering that uses value={form.team} and onChange={updateField}) to pass an explicit accessible label—prefer aria-label="Team Members" (or label/aria-labelledby if TagInput's API expects those) so the input is announced on focus; if TagInput requires an id/label pairing, add a matching id to the underlying input via TagInput props and reference it with aria-labelledby.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/synthorg/api/channels.py`:
- Around line 101-103: The except Exception branch in
src/synthorg/api/channels.py currently re-raises without logging; update the
generic except block (adjacent to the existing "except MemoryError,
RecursionError: raise" clause) to log a structured error before re-raising —
call logger.error or logger.exception with contextual fields (e.g., channel id,
message/payload, function name) and include the traceback (exc_info=True) so
observability is preserved, then re-raise the exception; do not change the
MemoryError/RecursionError behavior.
In `@src/synthorg/api/controllers/artifacts.py`:
- Around line 92-127: The helper _save_metadata_with_rollback currently types
repo and storage as Any; define small Protocols (e.g., ArtifactRepository with
async def save(self, artifact: Artifact) -> None and StorageBackend with async
def delete(self, artifact_id: str) -> None) or use existing repository/storage
interfaces, import typing.Protocol, then change the function signature to repo:
ArtifactRepository and storage: StorageBackend (and add any necessary
imports/exports); update tests/type hints as needed so repo.save and
storage.delete are properly typed.
In `@src/synthorg/api/controllers/projects.py`:
- Around line 156-166: After saving the project (repo.save(project)), make the
WebSocket publish a best-effort operation by wrapping the call to
publish_ws_event(...) in a try/except that catches Exception, logs the error
(including context: project.id and event type) and does not re-raise so the HTTP
response remains successful; locate the publish_ws_event call and modify it
accordingly (e.g., try: publish_ws_event(...) except Exception as e:
logger.error(...)) to ensure failures in publish_ws_event do not break the API
flow.
In `@web/src/components/ui/metadata-grid.tsx`:
- Line 34: The dt element in metadata-grid.tsx uses a raw utility class
"text-[10px]" which bypasses the project's typography tokens; replace that
ad-hoc size with the appropriate design-token class used across shared UI (i.e.,
swap "text-[10px]" in the dt className to the project tokenized size class used
for small/caption text in shared components—use the existing token (e.g., the
project's small/caption token) so the metadata-grid component conforms to the
tokenized typography system.
In `@web/src/pages/projects/ProjectDetailSkeleton.tsx`:
- Line 8: The className currently hardcodes a pixel breakpoint ("grid
grid-cols-2 gap-grid-gap max-[1023px]:grid-cols-1"); replace the arbitrary
max-[1023px] utility with a standard Tailwind breakpoint by making the default
single-column and enabling two columns at the lg breakpoint (e.g., change to
"grid grid-cols-1 gap-grid-gap lg:grid-cols-2") so the layout uses the
design-system breakpoints; update the JSX in ProjectDetailSkeleton's div
containing the grid accordingly.
In `@web/src/pages/projects/ProjectHeader.tsx`:
- Around line 18-20: The Budget field in ProjectHeader currently treats zero as
missing by using project.budget > 0; change the condition to check for
undefined/null instead so a legitimate 0 displays: update the rendering logic
that uses project.budget (and the value expression that calls formatCurrency) to
show formatCurrency(project.budget) when project.budget is defined (or non-null)
and only show '--' when project.budget is null/undefined, leaving formatCurrency
and the label 'Budget' intact.
In `@web/src/pages/projects/ProjectsSkeleton.tsx`:
- Line 11: The responsive grid in ProjectsSkeleton.tsx uses hardcoded pixel
max-queries (max-[1279px], max-[767px]); replace them with standard Tailwind
breakpoints by switching the class on the grid div (the <div className="grid
...">) to use breakpoint modifiers such as "grid-cols-1 md:grid-cols-2
xl:grid-cols-3 gap-grid-gap" (or equivalent order you prefer) so the layout uses
md and xl breakpoints instead of the custom max-[] values.
In `@web/src/stores/artifacts.ts`:
- Around line 138-141: The updateFromWsEvent handler currently declares an
unused event parameter; update the signature for updateFromWsEvent to either
remove the unused event parameter or rename it to _event to indicate intentional
non-use, and ensure any references to
useArtifactsStore.getState().fetchArtifacts() remain unchanged; apply the same
convention as used in the projects store so the pattern is consistent across
stores.
In `@web/src/stores/projects.ts`:
- Around line 119-121: The implementation of updateFromWsEvent currently ignores
the declared event: WsEvent parameter; either consume the event to decide when
to refetch (e.g., inspect event.type or event.resource and call
useProjectsStore.getState().fetchProjects() only for relevant event types) or
update the declaration/implementation to remove the unused parameter and/or add
a clear comment explaining that all events intentionally trigger a full refetch;
make the change in the updateFromWsEvent function to keep signature and behavior
consistent with the interface and to avoid unused-parameter warnings.
---
Duplicate comments:
In `@web/src/components/ui/metadata-grid.stories.tsx`:
- Around line 13-80: Add three stories named Hover, Loading, and Error alongside
the existing exports (Default, TwoColumns, FourColumns, WithMonoValues,
SingleItem, ManyItems, Empty). For Loading export a Story that sets a loading
prop (e.g., args: { isLoading: true } or items: undefined) to show the
spinner/placeholder state; for Error export a Story with an error prop (e.g.,
args: { error: 'Failed to load' }) to render the component's error UI; for Hover
export a Story that provides normal args (reuse Default.items) and a play
function that simulates a hover over the first metadata item (use
testing-library userEvent.hover on the component's root or first label) so the
hover visual state is captured. Ensure the new exports are named exactly Hover,
Loading, and Error to match storybook expectations.
In `@web/src/pages/projects/ProjectCreateDrawer.tsx`:
- Around line 103-111: The visible "Team Members" text is not programmatically
bound to the TagInput control, so assistive tech won't announce it; update the
TagInput usage in ProjectCreateDrawer (the TagInput rendering that uses
value={form.team} and onChange={updateField}) to pass an explicit accessible
label—prefer aria-label="Team Members" (or label/aria-labelledby if TagInput's
API expects those) so the input is announced on focus; if TagInput requires an
id/label pairing, add a matching id to the underlying input via TagInput props
and reference it with aria-labelledby.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b0ec860-3a5c-4c1e-9fb2-9aafc6318bd6
📒 Files selected for processing (63)
docs/design/brand-and-ux.mddocs/design/page-structure.mdsrc/synthorg/api/channels.pysrc/synthorg/api/controllers/artifacts.pysrc/synthorg/api/controllers/projects.pysrc/synthorg/api/dto.pysrc/synthorg/api/ws_models.pysrc/synthorg/core/artifact.pysrc/synthorg/core/project.pytests/unit/api/test_channels.pyweb/CLAUDE.mdweb/src/__tests__/helpers/factories.tsweb/src/__tests__/pages/ArtifactDetailPage.test.tsxweb/src/__tests__/pages/ArtifactsPage.test.tsxweb/src/__tests__/pages/ProjectDetailPage.test.tsxweb/src/__tests__/pages/ProjectsPage.test.tsxweb/src/__tests__/stores/artifacts.test.tsweb/src/__tests__/stores/projects.test.tsweb/src/api/endpoints/artifacts.tsweb/src/api/endpoints/projects.tsweb/src/api/types.tsweb/src/components/layout/Sidebar.tsxweb/src/components/ui/content-type-badge.stories.tsxweb/src/components/ui/content-type-badge.tsxweb/src/components/ui/metadata-grid.stories.tsxweb/src/components/ui/metadata-grid.tsxweb/src/components/ui/project-status-badge.stories.tsxweb/src/components/ui/project-status-badge.tsxweb/src/hooks/useArtifactDetailData.tsweb/src/hooks/useArtifactsData.tsweb/src/hooks/useDetailData.tsweb/src/hooks/useProjectDetailData.tsweb/src/hooks/useProjectsData.tsweb/src/mocks/handlers/artifacts.tsweb/src/mocks/handlers/index.tsweb/src/mocks/handlers/projects.tsweb/src/pages/ArtifactDetailPage.tsxweb/src/pages/ArtifactsPage.tsxweb/src/pages/ProjectDetailPage.tsxweb/src/pages/ProjectsPage.tsxweb/src/pages/artifacts/ArtifactCard.tsxweb/src/pages/artifacts/ArtifactContentPreview.tsxweb/src/pages/artifacts/ArtifactDetailSkeleton.tsxweb/src/pages/artifacts/ArtifactFilters.tsxweb/src/pages/artifacts/ArtifactGridView.tsxweb/src/pages/artifacts/ArtifactMetadata.tsxweb/src/pages/artifacts/ArtifactsSkeleton.tsxweb/src/pages/projects/ProjectCard.tsxweb/src/pages/projects/ProjectCreateDrawer.stories.tsxweb/src/pages/projects/ProjectCreateDrawer.tsxweb/src/pages/projects/ProjectDetailSkeleton.tsxweb/src/pages/projects/ProjectFilters.tsxweb/src/pages/projects/ProjectGridView.tsxweb/src/pages/projects/ProjectHeader.tsxweb/src/pages/projects/ProjectTaskList.tsxweb/src/pages/projects/ProjectTeamSection.tsxweb/src/pages/projects/ProjectsSkeleton.tsxweb/src/router/index.tsxweb/src/router/routes.tsweb/src/stores/artifacts.tsweb/src/stores/projects.tsweb/src/utils/download.tsweb/src/utils/format.ts
| except MemoryError, RecursionError: | ||
| raise | ||
| except Exception: |
There was a problem hiding this comment.
Log fatal publish failures before re-raising.
This branch re-raises without context, which drops observability for the most severe WS publish failures. Add a structured logger.error(...) before raise.
As per coding guidelines, "All error paths must log at WARNING or ERROR with context before raising."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/api/channels.py` around lines 101 - 103, The except Exception
branch in src/synthorg/api/channels.py currently re-raises without logging;
update the generic except block (adjacent to the existing "except MemoryError,
RecursionError: raise" clause) to log a structured error before re-raising —
call logger.error or logger.exception with contextual fields (e.g., channel id,
message/payload, function name) and include the traceback (exc_info=True) so
observability is preserved, then re-raise the exception; do not change the
MemoryError/RecursionError behavior.
| async def _save_metadata_with_rollback( | ||
| repo: Any, | ||
| storage: Any, | ||
| artifact_id: str, | ||
| updated: Artifact, | ||
| ) -> None: | ||
| """Save updated artifact metadata, rolling back storage on failure. | ||
|
|
||
| Args: | ||
| repo: Artifact persistence repository. | ||
| storage: Artifact content storage backend. | ||
| artifact_id: Artifact identifier. | ||
| updated: Updated artifact model. | ||
|
|
||
| Raises: | ||
| PersistenceError: If the metadata save fails (after rollback attempt). | ||
| """ | ||
| try: | ||
| await repo.save(updated) | ||
| except PersistenceError as exc: | ||
| logger.warning( | ||
| PERSISTENCE_ARTIFACT_SAVE_FAILED, | ||
| artifact_id=artifact_id, | ||
| error=str(exc), | ||
| note="metadata save failed, rolling back content", | ||
| ) | ||
| try: | ||
| await storage.delete(artifact_id) | ||
| except Exception as cleanup_exc: | ||
| logger.warning( | ||
| PERSISTENCE_ARTIFACT_STORAGE_ROLLBACK_FAILED, | ||
| artifact_id=artifact_id, | ||
| error=str(cleanup_exc), | ||
| ) | ||
| raise | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding type hints for repo and storage parameters.
The _save_metadata_with_rollback helper uses Any for repo and storage parameters. Adding proper type hints would improve type safety and IDE support:
async def _save_metadata_with_rollback(
- repo: Any,
- storage: Any,
+ repo: ArtifactRepository,
+ storage: ArtifactStorage,
artifact_id: str,
updated: Artifact,
) -> None:This assumes these types exist in the codebase. If not, protocol types could be defined for the required methods (save, delete, store).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/api/controllers/artifacts.py` around lines 92 - 127, The helper
_save_metadata_with_rollback currently types repo and storage as Any; define
small Protocols (e.g., ArtifactRepository with async def save(self, artifact:
Artifact) -> None and StorageBackend with async def delete(self, artifact_id:
str) -> None) or use existing repository/storage interfaces, import
typing.Protocol, then change the function signature to repo: ArtifactRepository
and storage: StorageBackend (and add any necessary imports/exports); update
tests/type hints as needed so repo.save and storage.delete are properly typed.
| publish_ws_event( | ||
| request, | ||
| WsEventType.PROJECT_CREATED, | ||
| CHANNEL_PROJECTS, | ||
| { | ||
| "project_id": project.id, | ||
| "name": project.name, | ||
| "status": project.status.value, | ||
| "lead": project.lead, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Guard WebSocket publish so project creation remains best-effort.
At Line [156], an exception from publish_ws_event(...) would fail the HTTP request after repo.save(project) already succeeded, causing false client failure and risky retries.
Suggested fix
await repo.save(project)
logger.info(PERSISTENCE_PROJECT_SAVED, project_id=project.id)
- publish_ws_event(
- request,
- WsEventType.PROJECT_CREATED,
- CHANNEL_PROJECTS,
- {
- "project_id": project.id,
- "name": project.name,
- "status": project.status.value,
- "lead": project.lead,
- },
- )
+ try:
+ publish_ws_event(
+ request,
+ WsEventType.PROJECT_CREATED,
+ CHANNEL_PROJECTS,
+ {
+ "project_id": project.id,
+ "name": project.name,
+ "status": project.status.value,
+ "lead": project.lead,
+ },
+ )
+ except Exception as exc:
+ logger.warning(
+ "api.projects.ws_publish_failed",
+ project_id=project.id,
+ event=WsEventType.PROJECT_CREATED.value,
+ error=str(exc),
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/api/controllers/projects.py` around lines 156 - 166, After
saving the project (repo.save(project)), make the WebSocket publish a
best-effort operation by wrapping the call to publish_ws_event(...) in a
try/except that catches Exception, logs the error (including context: project.id
and event type) and does not re-raise so the HTTP response remains successful;
locate the publish_ws_event call and modify it accordingly (e.g., try:
publish_ws_event(...) except Exception as e: logger.error(...)) to ensure
failures in publish_ws_event do not break the API flow.
| > | ||
| {items.map((item) => ( | ||
| <div key={item.label}> | ||
| <dt className="text-[10px] uppercase tracking-wide text-text-muted"> |
There was a problem hiding this comment.
Avoid arbitrary pixel typography in shared UI component.
text-[10px] bypasses the tokenized typography system for web/src/components/ui/*. Use a tokenized text size class instead.
♻️ Suggested change
- <dt className="text-[10px] uppercase tracking-wide text-text-muted">
+ <dt className="text-xs uppercase tracking-wide text-text-muted">As per coding guidelines, "When creating new shared components ... use design tokens exclusively."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <dt className="text-[10px] uppercase tracking-wide text-text-muted"> | |
| <dt className="text-xs uppercase tracking-wide text-text-muted"> | |
| {item.label} | |
| </dt> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/ui/metadata-grid.tsx` at line 34, The dt element in
metadata-grid.tsx uses a raw utility class "text-[10px]" which bypasses the
project's typography tokens; replace that ad-hoc size with the appropriate
design-token class used across shared UI (i.e., swap "text-[10px]" in the dt
className to the project tokenized size class used for small/caption text in
shared components—use the existing token (e.g., the project's small/caption
token) so the metadata-grid component conforms to the tokenized typography
system.
- Replace hardcoded pixel breakpoints in ProjectDetailSkeleton and ProjectsSkeleton with standard Tailwind tokens (lg:, sm:, xl:) - Fix ProjectHeader budget display: show formatCurrency(0) for zero budget instead of '--' (zero is a valid budget value) - Document intentional unused event parameter in updateFromWsEvent for both artifacts and projects stores with explanatory comment - Fix property-based test error string generators to use fc.lorem() instead of fc.string() to avoid whitespace edge cases in queryByText Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds full artifacts/projects management UX to the web dashboard and wires corresponding backend WebSocket channels/events to keep UI data fresh (polling + WS-triggered refetch), along with supporting shared UI components, mocks, and tests.
Changes:
- Backend: add
artifacts/projectsWS channels + event types and best-effort publishing on create/delete/upload mutations. - Frontend: add routes/pages, typed API endpoints + Zustand stores/hooks for artifacts/projects list/detail UIs (with polling + WS bindings).
- UI/tooling: add shared badges/metadata grid components (with Storybook), MSW handlers, and Vitest coverage for new stores/pages.
Reviewed changes
Copilot reviewed 63 out of 63 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/utils/format.ts | Adds formatFileSize() for artifact size display. |
| web/src/utils/download.ts | Adds helper to download artifact content as a file with toast-on-error. |
| web/src/stores/projects.ts | New Zustand store for projects list/detail state and actions. |
| web/src/stores/artifacts.ts | New Zustand store for artifacts list/detail state + text preview + delete. |
| web/src/router/routes.ts | Adds route constants for projects/artifacts list + detail. |
| web/src/router/index.tsx | Registers new lazy-loaded routes for projects/artifacts pages. |
| web/src/pages/ProjectsPage.tsx | Projects list page shell (counts, errors, WS warning, create drawer). |
| web/src/pages/projects/ProjectTeamSection.tsx | Team section UI for project detail page. |
| web/src/pages/projects/ProjectTaskList.tsx | Linked task list UI for project detail page. |
| web/src/pages/projects/ProjectsSkeleton.tsx | Loading skeleton for projects list page. |
| web/src/pages/projects/ProjectHeader.tsx | Project detail header using MetadataGrid + status badge. |
| web/src/pages/projects/ProjectGridView.tsx | Project card grid + empty state. |
| web/src/pages/projects/ProjectFilters.tsx | Client-side filters for projects list (search + status). |
| web/src/pages/projects/ProjectDetailSkeleton.tsx | Loading skeleton for project detail page. |
| web/src/pages/projects/ProjectCreateDrawer.tsx | Create project drawer + validation + submit wiring. |
| web/src/pages/projects/ProjectCreateDrawer.stories.tsx | Storybook stories for create project drawer. |
| web/src/pages/projects/ProjectCard.tsx | Project list card UI. |
| web/src/pages/ProjectDetailPage.tsx | Project detail page shell with error/WS banners and sections. |
| web/src/pages/ArtifactsPage.tsx | Artifacts list page shell (counts, errors, WS warning). |
| web/src/pages/artifacts/ArtifactsSkeleton.tsx | Loading skeleton for artifacts list page. |
| web/src/pages/artifacts/ArtifactMetadata.tsx | Artifact detail metadata + download/delete actions. |
| web/src/pages/artifacts/ArtifactGridView.tsx | Artifact card grid + empty state. |
| web/src/pages/artifacts/ArtifactFilters.tsx | Client-side filters for artifacts list. |
| web/src/pages/artifacts/ArtifactDetailSkeleton.tsx | Loading skeleton for artifact detail page. |
| web/src/pages/artifacts/ArtifactContentPreview.tsx | Inline content preview (text via CodeMirror, images via blob URL). |
| web/src/pages/artifacts/ArtifactCard.tsx | Artifact list card UI (size, content type, created time). |
| web/src/pages/ArtifactDetailPage.tsx | Artifact detail page shell with error/WS banners and sections. |
| web/src/mocks/handlers/projects.ts | Adds MSW handlers for project list + get-by-id. |
| web/src/mocks/handlers/index.ts | Exports new artifacts/projects MSW handler groups. |
| web/src/mocks/handlers/artifacts.ts | Adds MSW handlers for artifact list + get-by-id. |
| web/src/hooks/useProjectsData.ts | Projects list data hook (initial fetch, polling, WS refetch debounce, filtering). |
| web/src/hooks/useProjectDetailData.ts | Project detail data hook using shared useDetailData. |
| web/src/hooks/useDetailData.ts | Shared detail hook for polling + WS debounce + cleanup. |
| web/src/hooks/useArtifactsData.ts | Artifacts list data hook (initial fetch, polling, WS refetch debounce, filtering). |
| web/src/hooks/useArtifactDetailData.ts | Artifact detail data hook using shared useDetailData. |
| web/src/components/ui/project-status-badge.tsx | New semantic status-dot badge for project status. |
| web/src/components/ui/project-status-badge.stories.tsx | Storybook stories for ProjectStatusBadge. |
| web/src/components/ui/metadata-grid.tsx | New reusable key/value metadata grid component. |
| web/src/components/ui/metadata-grid.stories.tsx | Storybook stories for MetadataGrid. |
| web/src/components/ui/content-type-badge.tsx | New MIME-type badge for artifacts. |
| web/src/components/ui/content-type-badge.stories.tsx | Storybook stories for ContentTypeBadge. |
| web/src/components/layout/Sidebar.tsx | Adds sidebar nav entries for Projects and Artifacts. |
| web/src/api/types.ts | Adds Artifact/Project types, enums/value lists, and WS channels/event types. |
| web/src/api/endpoints/projects.ts | Implements typed listProjects, getProject, createProject. |
| web/src/api/endpoints/artifacts.ts | Implements typed list/get/create/delete + content download/text endpoints. |
| web/src/tests/stores/projects.test.ts | Store unit tests for projects state/actions/WS update behavior. |
| web/src/tests/stores/artifacts.test.ts | Store unit tests for artifacts state/actions/preview/delete/WS update behavior. |
| web/src/tests/pages/ProjectsPage.test.tsx | Page tests for Projects list page rendering states. |
| web/src/tests/pages/ProjectDetailPage.test.tsx | Page tests for Project detail page rendering states + invariants. |
| web/src/tests/pages/ArtifactsPage.test.tsx | Page tests for Artifacts list page rendering states + invariants. |
| web/src/tests/pages/ArtifactDetailPage.test.tsx | Page tests for Artifact detail page rendering states + invariants. |
| web/src/tests/helpers/factories.ts | Adds artifact/project factories for tests. |
| web/CLAUDE.md | Updates web architecture/component inventory docs for new domains/components. |
| tests/unit/api/test_channels.py | Updates channel test coverage for new WS channels and plugin config assertions. |
| src/synthorg/core/project.py | Updates project budget documentation strings. |
| src/synthorg/core/artifact.py | Adds project_id field and expands artifact attribute docs. |
| src/synthorg/api/ws_models.py | Extends WS event types; adjusts payload typing. |
| src/synthorg/api/dto.py | Extends CreateArtifactRequest with project_id; adds CreateProjectRequest validation. |
| src/synthorg/api/controllers/projects.py | Publishes project.created WS events on project creation. |
| src/synthorg/api/controllers/artifacts.py | Publishes artifact WS events on create/delete/upload; refactors rollback helper; adds project_id wiring. |
| src/synthorg/api/channels.py | Adds artifacts/projects channels + a shared best-effort publish_ws_event() helper. |
| docs/design/page-structure.md | Documents new pages/routes and WS channel mapping for artifacts/projects. |
| docs/design/brand-and-ux.md | Documents new shared UI components and formatFileSize() utility. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| event.model_dump_json(), | ||
| channels=[channel], | ||
| ) | ||
| except MemoryError, RecursionError: |
There was a problem hiding this comment.
except MemoryError, RecursionError: is invalid Python 3 syntax and will raise a SyntaxError at import time. Use except (MemoryError, RecursionError): (or separate except blocks) to preserve the intended re-raise behavior.
| except MemoryError, RecursionError: | |
| except (MemoryError, RecursionError): |
| <InputField | ||
| label="Deadline" | ||
| type="date" | ||
| value={form.deadline} | ||
| onChange={(e) => updateField('deadline', e.target.value)} |
There was a problem hiding this comment.
The form uses <input type="date">, which produces a YYYY-MM-DD string, but the backend CreateProjectRequest validates deadline with datetime.fromisoformat() (expects a full ISO datetime). This will cause project creation to fail whenever a deadline is set; convert the date to a valid ISO 8601 datetime (e.g., append a time/offset) or switch the input to collect a full ISO datetime.
| http.get('/api/v1/projects/:id', ({ params }) => { | ||
| const project = mockProjects.find((p) => p.id === params.id) | ||
| if (!project) { | ||
| return HttpResponse.json(apiSuccess(null), { status: 404 }) | ||
| } |
There was a problem hiding this comment.
The 404 branch returns apiSuccess(null) with success: true, which contradicts the error status and can break client-side error handling/unwrap semantics. Return apiError('Project not found') (with success: false) for consistency with other handlers (e.g. artifacts) and the ApiResponse contract.
| { label: 'Path', value: artifact.path, valueClassName: 'font-mono text-xs break-all' }, | ||
| { label: 'Task', value: artifact.task_id, valueClassName: 'font-mono text-xs' }, | ||
| { label: 'Created By', value: artifact.created_by }, | ||
| { label: 'Created', value: formatDate(artifact.created_at) }, | ||
| ] |
There was a problem hiding this comment.
artifact.project_id is part of the artifact model but isn't shown anywhere in the metadata grid. The PR description/docs call out a project link/field on the artifact detail page; consider adding a “Project” row (and optionally linking to the project detail route when present) so users can see the association.
| contentType.startsWith('text/') || | ||
| contentType === 'application/json' || | ||
| contentType === 'application/yaml' || | ||
| contentType === 'application/x-yaml' | ||
| ) |
There was a problem hiding this comment.
isPreviewableText() treats application/yaml as previewable, but the CodeMirror language selection (getLanguage()) only recognizes application/x-yaml / text/yaml. This means YAML content labeled as application/yaml will fall back to JSON mode; either include application/yaml in getLanguage() or remove it from the previewable set to keep behavior consistent.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/stores/artifacts.ts`:
- Around line 81-83: In fetchArtifactDetail, clear stale detail state when a new
fetch begins and when it fails: set selectedArtifact and contentPreview to null
alongside setting detailLoading: true and detailError: null at the start of the
function (before awaiting getArtifact(id)), and likewise clear
selectedArtifact/contentPreview when handling the error path before setting
detailError; keep using the existing _detailRequestId guard so only responses
matching the current request restore state.
- Around line 51-52: Replace the current string-based _detailRequestId
stale-guard with a monotonic request token: make _detailRequestId a numeric
counter (or generate a unique monotonic token) and, in each method that fetches
artifact details (the functions that currently set/read _detailRequestId and
compare with artifact id at the stale-guard sites), increment/assign the token
before the async request, capture it in a local variable, and after the await
compare that local token to the shared _detailRequestId before mutating state;
update every occurrence that currently uses artifact id for staleness (the sites
referencing _detailRequestId at the lines noted) so all overlapping requests for
the same artifact are guarded by the monotonic token check.
- Around line 113-119: deleteArtifact currently removes the artifact from
artifacts and decrements totalArtifacts but leaves selectedArtifact intact;
update the deleteArtifact flow (the deleteArtifact function) to also clear or
unset selectedArtifact when the deleted id matches state.selectedArtifact?.id
(e.g., set selectedArtifact to null/undefined) so the store does not retain
deleted detail data after the artifacts and totalArtifacts update.
In `@web/src/stores/projects.ts`:
- Around line 53-60: The fetchProjects() handler applies every async response
and conflicts with the optimistic-create prepend in lines around 97-99, causing
duplicates or lost items; introduce a request token (e.g., _listRequestToken)
incremented on each fetch and capture its value in the fetchProjects call, then
only apply result.data to the store if the captured token equals the current
_listRequestToken, and update createProject/optimistic prepend logic to either
use the same token check or re-run a safe merge only when the tokens match so
out-of-order fetches do not overwrite the optimistic state.
- Around line 36-37: fetchProjectDetail currently uses a single _detailRequestId
string and only compares project id, allowing stale slower responses to
overwrite newer data and never clearing selectedProject; change
fetchProjectDetail to generate a unique request token (e.g., a UUID or
incremented counter assigned to _detailRequestId) at the start of the load,
store that token locally in the async scope, and on every state update only
apply results if the stored token matches _detailRequestId (not just project
id); additionally clear selectedProject (set to null/undefined) when beginning a
new load and also on error/early-return so ProjectDetailPage doesn't render
stale data; update all similar blocks (lines around 63-78 and 109-116) that use
_detailRequestId to follow the same token-check-and-clear pattern.
- Around line 56-57: The code currently hard-caps project list and linked tasks
(see listProjects call that passes limit: 200 and the linked-tasks truncation at
50), which hides data; update the logic to remove fixed limits and implement
proper pagination/incremental loading: modify the listProjects usage (and any
getProjectDetails/getLinkedTasks helper) to accept page/limit parameters or to
fetch until totalProjects is reached (using result.total) instead of a single
fixed 200, and change the linked tasks path (the function that truncates to 50)
to request paged task results and expose a “show more” or load-more mechanism in
the store so the UI can load additional pages on demand. Ensure you use the
existing result.total/totalProjects and the store methods (set, listProjects,
getLinkedTasks/getProjectDetails) to drive incremental fetches rather than
hard-coded constants.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6ef5cf78-772d-4c64-85f1-e16a1bfe2084
📒 Files selected for processing (8)
web/src/__tests__/pages/ArtifactDetailPage.test.tsxweb/src/__tests__/pages/ArtifactsPage.test.tsxweb/src/__tests__/pages/ProjectDetailPage.test.tsxweb/src/pages/projects/ProjectDetailSkeleton.tsxweb/src/pages/projects/ProjectHeader.tsxweb/src/pages/projects/ProjectsSkeleton.tsxweb/src/stores/artifacts.tsweb/src/stores/projects.ts
📜 Review details
⏰ 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). (8)
- GitHub Check: Agent
- GitHub Check: Dashboard Test
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Sandbox
- GitHub Check: Build Web
- GitHub Check: Build Backend
- GitHub Check: Dependency Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (1)
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx}: Always reuse existing components fromweb/src/components/ui/before creating new ones, including StatusBadge, MetricCard, Sparkline, SectionCard, AgentCard, DeptHealthBar, ProgressGauge, StatPill, Avatar, Button, Toast/ToastContainer, Skeleton variants, EmptyState, ErrorBoundary, ConfirmDialog, CommandPalette, InlineEdit, AnimatedPresence, StaggerGroup/StaggerItem, Drawer, InputField, SelectField, SliderField, ToggleField, TaskStatusIndicator, PriorityBadge, ProviderHealthBadge, TokenUsageBar, CodeMirrorEditor, SegmentedControl, ThemeToggle, LiveRegion, MobileUnsupportedOverlay, LazyCodeMirrorEditor, and TagInput
Use Tailwind semantic classes (text-foreground, bg-card, text-accent, text-success, bg-danger, etc.) or CSS variables (var(--so-accent)) for colors; NEVER hardcode hex values in .tsx/.ts files
Use font-sans or font-mono (maps to Geist tokens) for typography; NEVER set fontFamily directly
Use density-aware tokens (p-card, gap-section-gap, gap-grid-gap) or standard Tailwind spacing; NEVER hardcode pixel values for layout spacing
Use token variables (var(--so-shadow-card-hover), border-border, border-bright) for shadows and borders; NEVER hardcode values
Do NOT recreate status dots inline -- use
Do NOT build card-with-header layouts from scratch -- use
Do NOT create metric displays with text-metric font-bold -- use
Do NOT render initials circles manually -- use
Do NOT create complex (>8 line) JSX inside .map() -- extract to a shared component
Do NOT use rgba() with hardcoded values -- use design token variables
Do NOT use hardcoded Framer Motion transition durations -- use presets from@/lib/motion
web/src/**/*.{ts,tsx}: Apply eslint-web (zero warnings) via pre-commit hooks for the web dashboard (conditional onweb/src/**/*.{ts,tsx})
Always reuse existing components fromweb/src/components/ui/before creating new ones in the React dashboard
Never hardcode hex colors, font-famil...
Files:
web/src/pages/projects/ProjectsSkeleton.tsxweb/src/pages/projects/ProjectDetailSkeleton.tsxweb/src/pages/projects/ProjectHeader.tsxweb/src/__tests__/pages/ArtifactDetailPage.test.tsxweb/src/__tests__/pages/ArtifactsPage.test.tsxweb/src/__tests__/pages/ProjectDetailPage.test.tsxweb/src/stores/artifacts.tsweb/src/stores/projects.ts
🧠 Learnings (21)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Web dashboard: see `web/CLAUDE.md` for commands, design system, and component inventory
📚 Learning: 2026-03-31T14:41:06.345Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2026-03-31T14:41:06.345Z
Learning: Applies to web/src/components/ui/**/*.{ts,tsx} : When creating new shared components, place in web/src/components/ui/ with descriptive kebab-case filename, create a .stories.tsx file with all states (default, hover, loading, error, empty), export props as TypeScript interface, use design tokens exclusively, and import cn from `@/lib/utils` for conditional class merging
Applied to files:
web/src/pages/projects/ProjectsSkeleton.tsxweb/src/pages/projects/ProjectDetailSkeleton.tsx
📚 Learning: 2026-03-31T14:41:06.345Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2026-03-31T14:41:06.345Z
Learning: Applies to web/src/**/*.{ts,tsx} : Always reuse existing components from `web/src/components/ui/` before creating new ones, including StatusBadge, MetricCard, Sparkline, SectionCard, AgentCard, DeptHealthBar, ProgressGauge, StatPill, Avatar, Button, Toast/ToastContainer, Skeleton variants, EmptyState, ErrorBoundary, ConfirmDialog, CommandPalette, InlineEdit, AnimatedPresence, StaggerGroup/StaggerItem, Drawer, InputField, SelectField, SliderField, ToggleField, TaskStatusIndicator, PriorityBadge, ProviderHealthBadge, TokenUsageBar, CodeMirrorEditor, SegmentedControl, ThemeToggle, LiveRegion, MobileUnsupportedOverlay, LazyCodeMirrorEditor, and TagInput
Applied to files:
web/src/pages/projects/ProjectsSkeleton.tsxweb/src/pages/projects/ProjectDetailSkeleton.tsxweb/src/pages/projects/ProjectHeader.tsxweb/src/__tests__/pages/ArtifactDetailPage.test.tsxweb/src/__tests__/pages/ArtifactsPage.test.tsx
📚 Learning: 2026-03-31T22:07:03.782Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T22:07:03.782Z
Learning: Applies to web/src/**/*.{ts,tsx} : Always reuse existing components from `web/src/components/ui/` before creating new ones in the React dashboard
Applied to files:
web/src/pages/projects/ProjectsSkeleton.tsxweb/src/pages/projects/ProjectDetailSkeleton.tsx
📚 Learning: 2026-03-31T14:41:06.345Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2026-03-31T14:41:06.345Z
Learning: Applies to web/src/**/*.{ts,tsx} : Do NOT create complex (>8 line) JSX inside .map() -- extract to a shared component
Applied to files:
web/src/pages/projects/ProjectsSkeleton.tsx
📚 Learning: 2026-03-31T14:41:06.345Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2026-03-31T14:41:06.345Z
Learning: Applies to web/src/**/*.{ts,tsx} : Do NOT build card-with-header layouts from scratch -- use <SectionCard>
Applied to files:
web/src/pages/projects/ProjectsSkeleton.tsxweb/src/pages/projects/ProjectDetailSkeleton.tsxweb/src/pages/projects/ProjectHeader.tsx
📚 Learning: 2026-03-31T14:41:06.345Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2026-03-31T14:41:06.345Z
Learning: Applies to web/src/**/*.{ts,tsx} : Use density-aware tokens (p-card, gap-section-gap, gap-grid-gap) or standard Tailwind spacing; NEVER hardcode pixel values for layout spacing
Applied to files:
web/src/pages/projects/ProjectsSkeleton.tsxweb/src/pages/projects/ProjectDetailSkeleton.tsx
📚 Learning: 2026-03-31T14:41:06.345Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2026-03-31T14:41:06.345Z
Learning: Applies to web/src/**/*.{ts,tsx} : Use Tailwind semantic classes (text-foreground, bg-card, text-accent, text-success, bg-danger, etc.) or CSS variables (var(--so-accent)) for colors; NEVER hardcode hex values in .tsx/.ts files
Applied to files:
web/src/pages/projects/ProjectsSkeleton.tsxweb/src/pages/projects/ProjectDetailSkeleton.tsx
📚 Learning: 2026-03-31T22:07:03.782Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T22:07:03.782Z
Learning: Applies to web/src/**/*.{ts,tsx} : Never hardcode hex colors, font-family, pixel spacing, or Framer Motion transitions in the web dashboard — use design tokens and `@/lib/motion` presets
Applied to files:
web/src/pages/projects/ProjectsSkeleton.tsxweb/src/pages/projects/ProjectDetailSkeleton.tsx
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Applies to web/src/**/*.{ts,tsx} : NEVER hardcode hex colors, font-family, pixel spacing, or Framer Motion transitions — use design tokens and `@/lib/motion` presets
Applied to files:
web/src/pages/projects/ProjectsSkeleton.tsxweb/src/pages/projects/ProjectDetailSkeleton.tsx
📚 Learning: 2026-03-31T14:41:06.345Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2026-03-31T14:41:06.345Z
Learning: Applies to web/src/**/*.{ts,tsx} : Use token variables (var(--so-shadow-card-hover), border-border, border-bright) for shadows and borders; NEVER hardcode values
Applied to files:
web/src/pages/projects/ProjectsSkeleton.tsxweb/src/pages/projects/ProjectDetailSkeleton.tsx
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Applies to web/src/**/*.{ts,tsx} : Use React 19, TypeScript 6.0+, and design system tokens from shadcn/ui + Tailwind CSS 4 + Radix UI in web dashboard
Applied to files:
web/src/pages/projects/ProjectsSkeleton.tsxweb/src/pages/projects/ProjectDetailSkeleton.tsxweb/src/pages/projects/ProjectHeader.tsx
📚 Learning: 2026-03-20T08:28:32.845Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T08:28:32.845Z
Learning: Applies to web/src/__tests__/**/*.{ts,js} : Dashboard testing: Vitest unit tests organized by feature under `web/src/__tests__/`. Use fast-check for property-based testing (`fc.assert` + `fc.property`).
Applied to files:
web/src/__tests__/pages/ArtifactDetailPage.test.tsxweb/src/__tests__/pages/ArtifactsPage.test.tsxweb/src/__tests__/pages/ProjectDetailPage.test.tsx
📚 Learning: 2026-03-31T14:41:06.345Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2026-03-31T14:41:06.345Z
Learning: Applies to web/src/**/*.stories.tsx : Use storybook/test instead of storybook/test for Storybook 10 imports
Applied to files:
web/src/__tests__/pages/ArtifactDetailPage.test.tsxweb/src/__tests__/pages/ArtifactsPage.test.tsxweb/src/__tests__/pages/ProjectDetailPage.test.tsx
📚 Learning: 2026-03-31T14:41:06.345Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2026-03-31T14:41:06.345Z
Learning: A PostToolUse hook (scripts/check_web_design_system.py) runs automatically on every Edit/Write to web/src/ files and catches: hardcoded hex colors and rgba values, hardcoded font-family declarations, hardcoded Framer Motion transition durations, new components without Storybook stories, duplicate patterns, and complex .map() blocks. Fix all violations before proceeding.
Applied to files:
web/src/__tests__/pages/ArtifactDetailPage.test.tsxweb/src/__tests__/pages/ArtifactsPage.test.tsxweb/src/__tests__/pages/ProjectDetailPage.test.tsx
📚 Learning: 2026-03-31T14:41:06.345Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2026-03-31T14:41:06.345Z
Learning: Applies to web/.storybook/preview.{ts,tsx} : In Storybook preview.tsx, set parameters.a11y.test to 'error', 'todo', or 'off' to enforce WCAG compliance on all stories (replace old .element and .manual patterns)
Applied to files:
web/src/__tests__/pages/ArtifactDetailPage.test.tsxweb/src/__tests__/pages/ArtifactsPage.test.tsxweb/src/__tests__/pages/ProjectDetailPage.test.tsx
📚 Learning: 2026-03-16T07:22:28.134Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:22:28.134Z
Learning: Applies to tests/**/*.py : NEVER skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally. For timing-sensitive tests, mock `time.monotonic()` and `asyncio.sleep()` to make them deterministic instead of widening timing margins
Applied to files:
web/src/__tests__/pages/ArtifactDetailPage.test.tsxweb/src/__tests__/pages/ArtifactsPage.test.tsx
📚 Learning: 2026-03-31T22:07:03.782Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T22:07:03.782Z
Learning: Applies to tests/**/*.py : Never skip, dismiss, or ignore flaky tests. Always fix them fully and fundamentally. For timing-sensitive tests, mock `time.monotonic()` and `asyncio.sleep()` to make them deterministic. For indefinite blocking tasks, use `asyncio.Event().wait()` instead of `asyncio.sleep(large_number)`
Applied to files:
web/src/__tests__/pages/ArtifactDetailPage.test.tsx
📚 Learning: 2026-03-31T22:07:03.782Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T22:07:03.782Z
Learning: Applies to web/src/**/*.{ts,tsx} : The PostToolUse hook (`scripts/check_web_design_system.py`) enforces design system rules (component reuse, design token usage) on every Edit/Write to `web/src/`
Applied to files:
web/src/__tests__/pages/ProjectDetailPage.test.tsx
📚 Learning: 2026-03-15T21:49:53.264Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:49:53.264Z
Learning: Fix everything valid — never skip when review agents find valid issues (including pre-existing issues in surrounding code, suggestions, and findings adjacent to the PR's changes). No deferring, no 'out of scope' skipping.
Applied to files:
web/src/stores/projects.ts
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Fix all valid issues found by review agents (including pre-existing issues in surrounding code, suggestions, and adjacent findings) — never skip or defer
Applied to files:
web/src/stores/projects.ts
🔇 Additional comments (6)
web/src/pages/projects/ProjectsSkeleton.tsx (1)
1-18: Looks good — skeleton implementation is clean and guideline-aligned.Component reuse, responsive grid tokens, and map rendering are all in good shape.
web/src/pages/projects/ProjectDetailSkeleton.tsx (1)
1-13: Design-system compliant skeleton implementation.Good use of shared
Skeleton/SkeletonCardprimitives and tokenized responsive layout classes.As per coding guidelines, "Always reuse existing components from
web/src/components/ui/before creating new ones" and "Use density-aware tokens ... or standard Tailwind spacing; NEVER hardcode pixel values for layout spacing."web/src/pages/projects/ProjectHeader.tsx (1)
11-49: Header composition and metadata rendering look solid.This correctly uses shared components and preserves valid zero-budget display behavior.
As per coding guidelines, "Do NOT build card-with-header layouts from scratch -- use " and "Always reuse existing components from
web/src/components/ui/before creating new ones."web/src/__tests__/pages/ArtifactDetailPage.test.tsx (1)
84-130: Property-based state-transition coverage is well implemented.The randomized invariants are aligned with the page’s conditional render contract and complement the example-based cases well.
Based on learnings, "Dashboard testing: Vitest unit tests organized by feature under
web/src/__tests__/. Use fast-check for property-based testing (fc.assert+fc.property)."web/src/__tests__/pages/ArtifactsPage.test.tsx (1)
88-142: Strong invariant testing for page state behavior.The property tests are focused and correctly validate skeleton/error behavior across generated state permutations.
Based on learnings, "Dashboard testing: Vitest unit tests organized by feature under
web/src/__tests__/. Use fast-check for property-based testing (fc.assert+fc.property)."web/src/__tests__/pages/ProjectDetailPage.test.tsx (1)
88-134: Property-based transition checks are correctly scoped and useful.Nice addition of invariant-based checks for loading/not-found/error branches on top of scenario tests.
Based on learnings, "Dashboard testing: Vitest unit tests organized by feature under
web/src/__tests__/. Use fast-check for property-based testing (fc.assert+fc.property)."
| const result = await listProjects({ limit: 200 }) | ||
| set({ projects: result.data, totalProjects: result.total, listLoading: false }) |
There was a problem hiding this comment.
These fixed limits silently hide projects and tasks.
Line 56 hard-caps the dashboard to 200 loaded projects even though the store’s search/status/lead filters are local, so larger workspaces become partially unsearchable. Line 69 similarly truncates linked tasks at 50 with no pagination or “show more” signal, which means the project detail view can look complete while omitting data.
Also applies to: 67-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/stores/projects.ts` around lines 56 - 57, The code currently
hard-caps project list and linked tasks (see listProjects call that passes
limit: 200 and the linked-tasks truncation at 50), which hides data; update the
logic to remove fixed limits and implement proper pagination/incremental
loading: modify the listProjects usage (and any getProjectDetails/getLinkedTasks
helper) to accept page/limit parameters or to fetch until totalProjects is
reached (using result.total) instead of a single fixed 200, and change the
linked tasks path (the function that truncates to 50) to request paged task
results and expose a “show more” or load-more mechanism in the store so the UI
can load additional pages on demand. Ensure you use the existing
result.total/totalProjects and the store methods (set, listProjects,
getLinkedTasks/getProjectDetails) to drive incremental fetches rather than
hard-coded constants.
- Replace string-based _detailRequestId with monotonic counter token for more robust stale-response guards (handles same-ID refetch) - Clear selectedArtifact/contentPreview at fetch start and on error to prevent stale data flash during navigation - Clear selectedProject/projectTasks at fetch start and on error - deleteArtifact clears selectedArtifact when the deleted ID matches the currently viewed artifact - Extract isStaleRequest() helper to avoid false-positive timing attack lint warnings on token comparison Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
web/src/stores/projects.ts (3)
96-105:⚠️ Potential issue | 🟡 MinorOptimistic update can cause temporary duplicate entries.
If a WS-triggered
fetchProjects()resolves between the API call and the optimisticset(), or if the server already includes the new project in a concurrent response, the project may appear twice until the next reconciliation.🛠️ Proposed fix: deduplicate on insert
createProject: async (data: CreateProjectRequest) => { const project = await createProjectApi(data) - // Optimistically add to local state for immediate UI update - set((state) => ({ - projects: [project, ...state.projects], - totalProjects: state.totalProjects + 1, - })) + // Add to local state, avoiding duplicates if a refetch already included it + set((state) => { + if (state.projects.some((p) => p.id === project.id)) return state + return { + projects: [project, ...state.projects], + totalProjects: state.totalProjects + 1, + } + }) // Polling and WS events will reconcile with server state. return project },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/stores/projects.ts` around lines 96 - 105, The optimistic insert in createProject (which calls createProjectApi and then set(...)) can produce duplicates if a WS or fetchProjects() already added the same project; fix by deduplicating before inserting: when updating state in createProject's set callback, filter out any existing project with the same unique id (e.g., project.id) from state.projects and then prepend the new project, and adjust totalProjects only if the id was not already present; this ensures createProject, the set handler, and interactions with fetchProjects()/WS events won't produce temporary duplicates.
55-63:⚠️ Potential issue | 🟠 MajorList fetch race condition remains unaddressed.
Concurrent
fetchProjects()calls (from polling + WS events) can race, and the slower response overwrites newer data. UnlikefetchProjectDetail, this action has no request token guard.Additionally, the hard limit of 200 projects means larger workspaces will have data silently hidden from client-side search/filters.
🛠️ Proposed fix: add list request token
+let _listRequestToken = 0 fetchProjects: async () => { + const token = ++_listRequestToken set({ listLoading: true, listError: null }) try { const result = await listProjects({ limit: 200 }) + if (_listRequestToken !== token) return set({ projects: result.data, totalProjects: result.total, listLoading: false }) } catch (err) { + if (_listRequestToken !== token) return set({ listLoading: false, listError: getErrorMessage(err) }) } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/stores/projects.ts` around lines 55 - 63, fetchProjects can race when called concurrently (polling + WS) and the slower response can overwrite newer data; also the hard-coded limit of 200 hides projects for large workspaces. Fix by adding a request token guard: when fetchProjects starts, generate and save a new token into state (e.g., state key listRequestToken), capture it in the local scope, call listProjects (avoid forcing limit=200 — pass a configurable limit or omit to enable server-side paging), and on success only set projects/totalProjects/listLoading if the saved token still matches the captured token; ensure errors also check/reset listLoading only for the active token and set listError using getErrorMessage(err). Use the existing fetchProjects function and state keys (listRequestToken, projects, totalProjects, listLoading, listError) to implement this guard.
111-119:⚠️ Potential issue | 🔴 CriticalResetting token to 0 breaks stale-request detection.
Setting
_detailRequestToken = 0instead of incrementing it allows token collisions after navigation. PeruseDetailData(relevant snippet#4),clearDetail()is called on unmount. Race scenario:
- User views Project A →
fetchProjectDetail("A"): token = 1- User navigates to Project B → cleanup calls
clearDetail(): token = 0fetchProjectDetail("B")called: token = ++0 = 1 (same as A's token!)- Project A's response arrives first:
isStaleRequest(1)checks1 !== 1→ false → stale data applied- User sees Project A data on Project B's page until B's response overwrites it
🐛 Proposed fix: increment instead of reset
clearDetail: () => { - _detailRequestToken = 0 + _detailRequestToken += 1 set({ selectedProject: null, projectTasks: [], detailLoading: false, detailError: null, }) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/stores/projects.ts` around lines 111 - 119, clearDetail currently resets _detailRequestToken to 0 which allows token reuse and stale-response collisions; instead increment the token so any prior requests become stale. In the clearDetail function (referencing clearDetail and _detailRequestToken), replace the assignment _detailRequestToken = 0 with an increment (e.g., ++_detailRequestToken or _detailRequestToken++) so fetchProjectDetail / isStaleRequest logic (and the cleanup in useDetailData) will correctly mark earlier responses as stale.web/src/stores/artifacts.ts (1)
132-133:⚠️ Potential issue | 🟠 MajorRegression:
clearDetail()makes detail request tokens reusable again.Line 133 resets
_detailRequestTokento0. SinceuseArtifactDetailDatacallsclearDetail()on unmount, a fast leave/re-enter can reuse token1and let an olderfetchArtifactDetail()response passisStaleRequest(), briefly restoring the wrong artifact. Keep the counter monotonic here.♻️ Minimal fix
clearDetail: () => { - _detailRequestToken = 0 + _detailRequestToken++ set({ selectedArtifact: null, contentPreview: null, detailLoading: false, detailError: null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/stores/artifacts.ts` around lines 132 - 133, clearDetail() currently resets _detailRequestToken to 0 which makes tokens reusable and allows stale fetchArtifactDetail() responses to pass isStaleRequest() after a fast unmount/remount; instead make the token monotonic by bumping it (e.g. replace the reset with an increment such as ++_detailRequestToken) so every clearDetail() produces a newer token and older responses are always considered stale; update clearDetail in the artifacts store (referencing clearDetail and _detailRequestToken) — no other behavioral changes to useArtifactDetailData, fetchArtifactDetail, or isStaleRequest are needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/stores/artifacts.ts`:
- Around line 73-77: The fetchArtifacts function is hard-capping results by
calling listArtifacts({ limit: 200 }), preventing filters/pagination from
reaching the backend; update fetchArtifacts to stop using a fixed limit and
instead accept and forward active store filters and pagination parameters (e.g.,
page, pageSize, cursor) to listArtifacts (or implement server-side pagination
loop if you truly need all rows), so that listArtifacts receives the current
filters from the store and returns the correct subset and totalArtifacts; ensure
set calls update listLoading/listError appropriately while using the returned
paging metadata to populate artifacts and totalArtifacts.
---
Duplicate comments:
In `@web/src/stores/artifacts.ts`:
- Around line 132-133: clearDetail() currently resets _detailRequestToken to 0
which makes tokens reusable and allows stale fetchArtifactDetail() responses to
pass isStaleRequest() after a fast unmount/remount; instead make the token
monotonic by bumping it (e.g. replace the reset with an increment such as
++_detailRequestToken) so every clearDetail() produces a newer token and older
responses are always considered stale; update clearDetail in the artifacts store
(referencing clearDetail and _detailRequestToken) — no other behavioral changes
to useArtifactDetailData, fetchArtifactDetail, or isStaleRequest are needed.
In `@web/src/stores/projects.ts`:
- Around line 96-105: The optimistic insert in createProject (which calls
createProjectApi and then set(...)) can produce duplicates if a WS or
fetchProjects() already added the same project; fix by deduplicating before
inserting: when updating state in createProject's set callback, filter out any
existing project with the same unique id (e.g., project.id) from state.projects
and then prepend the new project, and adjust totalProjects only if the id was
not already present; this ensures createProject, the set handler, and
interactions with fetchProjects()/WS events won't produce temporary duplicates.
- Around line 55-63: fetchProjects can race when called concurrently (polling +
WS) and the slower response can overwrite newer data; also the hard-coded limit
of 200 hides projects for large workspaces. Fix by adding a request token guard:
when fetchProjects starts, generate and save a new token into state (e.g., state
key listRequestToken), capture it in the local scope, call listProjects (avoid
forcing limit=200 — pass a configurable limit or omit to enable server-side
paging), and on success only set projects/totalProjects/listLoading if the saved
token still matches the captured token; ensure errors also check/reset
listLoading only for the active token and set listError using
getErrorMessage(err). Use the existing fetchProjects function and state keys
(listRequestToken, projects, totalProjects, listLoading, listError) to implement
this guard.
- Around line 111-119: clearDetail currently resets _detailRequestToken to 0
which allows token reuse and stale-response collisions; instead increment the
token so any prior requests become stale. In the clearDetail function
(referencing clearDetail and _detailRequestToken), replace the assignment
_detailRequestToken = 0 with an increment (e.g., ++_detailRequestToken or
_detailRequestToken++) so fetchProjectDetail / isStaleRequest logic (and the
cleanup in useDetailData) will correctly mark earlier responses as stale.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d591c668-a141-4911-aa9e-a5d3fdf9948a
📒 Files selected for processing (2)
web/src/stores/artifacts.tsweb/src/stores/projects.ts
📜 Review details
⏰ 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). (7)
- GitHub Check: Dashboard Test
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Web
- GitHub Check: Build Sandbox
- GitHub Check: Build Backend
- GitHub Check: Dependency Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (1)
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx}: Always reuse existing components fromweb/src/components/ui/before creating new ones, including StatusBadge, MetricCard, Sparkline, SectionCard, AgentCard, DeptHealthBar, ProgressGauge, StatPill, Avatar, Button, Toast/ToastContainer, Skeleton variants, EmptyState, ErrorBoundary, ConfirmDialog, CommandPalette, InlineEdit, AnimatedPresence, StaggerGroup/StaggerItem, Drawer, InputField, SelectField, SliderField, ToggleField, TaskStatusIndicator, PriorityBadge, ProviderHealthBadge, TokenUsageBar, CodeMirrorEditor, SegmentedControl, ThemeToggle, LiveRegion, MobileUnsupportedOverlay, LazyCodeMirrorEditor, and TagInput
Use Tailwind semantic classes (text-foreground, bg-card, text-accent, text-success, bg-danger, etc.) or CSS variables (var(--so-accent)) for colors; NEVER hardcode hex values in .tsx/.ts files
Use font-sans or font-mono (maps to Geist tokens) for typography; NEVER set fontFamily directly
Use density-aware tokens (p-card, gap-section-gap, gap-grid-gap) or standard Tailwind spacing; NEVER hardcode pixel values for layout spacing
Use token variables (var(--so-shadow-card-hover), border-border, border-bright) for shadows and borders; NEVER hardcode values
Do NOT recreate status dots inline -- use
Do NOT build card-with-header layouts from scratch -- use
Do NOT create metric displays with text-metric font-bold -- use
Do NOT render initials circles manually -- use
Do NOT create complex (>8 line) JSX inside .map() -- extract to a shared component
Do NOT use rgba() with hardcoded values -- use design token variables
Do NOT use hardcoded Framer Motion transition durations -- use presets from@/lib/motion
web/src/**/*.{ts,tsx}: Apply eslint-web (zero warnings) via pre-commit hooks for the web dashboard (conditional onweb/src/**/*.{ts,tsx})
Always reuse existing components fromweb/src/components/ui/before creating new ones in the React dashboard
Never hardcode hex colors, font-famil...
Files:
web/src/stores/artifacts.tsweb/src/stores/projects.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:17:43.675Z
Learning: Applies to web/** : Web dashboard: Node.js 20+, dependencies in web/package.json (Vue 3, PrimeVue, Tailwind CSS, Pinia, VueFlow, ECharts, Axios, vue-draggable-plus, Vitest, fast-check, ESLint, vue-tsc).
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/api/**/*.py : API package (api/): Litestar REST + WebSocket with controllers, guards, channels, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint, provider management endpoint (CRUD + test + presets), backup endpoint, RFC 9457 structured errors, AppState hot-reload slots, service auto-wiring (Phase 1 at construction, Phase 2 on startup), lifecycle helpers
📚 Learning: 2026-03-15T21:49:53.264Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:49:53.264Z
Learning: Fix everything valid — never skip when review agents find valid issues (including pre-existing issues in surrounding code, suggestions, and findings adjacent to the PR's changes). No deferring, no 'out of scope' skipping.
Applied to files:
web/src/stores/projects.ts
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Fix all valid issues found by review agents (including pre-existing issues in surrounding code, suggestions, and adjacent findings) — never skip or defer
Applied to files:
web/src/stores/projects.ts
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Applies to {**/*.py,web/src/**/*.{ts,tsx}} : Keep functions under 50 lines and files under 800 lines
Applied to files:
web/src/stores/projects.ts
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Applies to {**/*.py,web/src/**/*.{ts,tsx}} : Line length: 88 characters (enforced by ruff)
Applied to files:
web/src/stores/projects.ts
🔇 Additional comments (4)
web/src/stores/projects.ts (4)
1-34: LGTM!The imports and interface are well-structured. Good use of
readonlyarrays for immutability and clear separation between list, detail, and filter state.
36-38: LGTM!Good use of a monotonic counter with an extracted helper function to centralize stale-request detection.
65-94: LGTM!Good implementation with proper stale-request guarding, state clearing at fetch start, and graceful handling of partial failures with informative error messages. The
Promise.allSettledusage correctly handles independent fetch failures.Note: The 50-task limit (line 71) may hide data for projects with many tasks. Consider adding pagination or a "show more" indicator in a future iteration.
121-125: LGTM!The comment clearly documents why the event payload is intentionally ignored. Given the 30s polling interval, a full refetch is a reasonable simplification.
| fetchArtifacts: async () => { | ||
| set({ listLoading: true, listError: null }) | ||
| try { | ||
| const result = await listArtifacts({ limit: 200 }) | ||
| set({ artifacts: result.data, totalArtifacts: result.total, listLoading: false }) |
There was a problem hiding this comment.
Stop truncating the artifact list at 200 rows.
Line 76 always calls listArtifacts({ limit: 200 }), so none of the store filters ever reach the API and the dashboard only searches/manages the first 200 artifacts loaded into memory. Once result.total > 200, matching artifacts after page 1 become impossible to find even though totalArtifacts still reports them. Please either paginate this store or send the active filters to the backend instead of hard-capping an unfiltered snapshot.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/stores/artifacts.ts` around lines 73 - 77, The fetchArtifacts
function is hard-capping results by calling listArtifacts({ limit: 200 }),
preventing filters/pagination from reaching the backend; update fetchArtifacts
to stop using a fixed limit and instead accept and forward active store filters
and pagination parameters (e.g., page, pageSize, cursor) to listArtifacts (or
implement server-side pagination loop if you truly need all rows), so that
listArtifacts receives the current filters from the store and returns the
correct subset and totalArtifacts; ensure set calls update listLoading/listError
appropriately while using the returned paging metadata to populate artifacts and
totalArtifacts.
- clearDetail increments token instead of resetting to 0, preventing stale-response collision on fast unmount/remount cycles - createProject deduplicates by ID before optimistic prepend to prevent transient duplicates when concurrent fetch returns same project - fetchProjects uses list request token guard so slower concurrent responses (polling + WS) do not overwrite newer data Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 63 out of 63 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| createProject: async (data: CreateProjectRequest) => { | ||
| const project = await createProjectApi(data) | ||
| // Optimistically add to local state for immediate UI update. | ||
| // Filter by ID first to prevent duplicates if a concurrent fetch already added it. | ||
| set((state) => { | ||
| const filtered = state.projects.filter((p) => p.id !== project.id) | ||
| return { | ||
| projects: [project, ...filtered], | ||
| totalProjects: filtered.length + 1, | ||
| } | ||
| }) |
There was a problem hiding this comment.
totalProjects is being set to filtered.length + 1 after createProject(). This can corrupt the count when the list is paginated/limited (e.g., limit: 200) or already contains more/less items than totalProjects. Consider incrementing from the previous state.totalProjects (while still de-duping the list) or reusing the server-reported total when available.
| function handleDownload() { | ||
| downloadArtifactFile(artifact.id, artifact.path.split('/').pop() ?? artifact.id) | ||
| } |
There was a problem hiding this comment.
Filename fallback uses artifact.path.split('/').pop() ?? artifact.id. If path ends with /, .pop() returns an empty string, and ?? will keep it, producing a blank download filename. Use || (or explicitly check for a non-empty string) so empty names fall back to artifact.id.
| function getLanguage(contentType: string): 'json' | 'yaml' { | ||
| const lower = contentType.toLowerCase() | ||
| if (lower === 'application/json') return 'json' | ||
| if (lower === 'application/x-yaml' || lower === 'text/yaml') return 'yaml' | ||
| // Falls back to JSON mode for non-JSON/non-YAML text types. | ||
| // Plain text may show minor syntax coloring. | ||
| return 'json' | ||
| } |
There was a problem hiding this comment.
getLanguage() treats application/json as JSON and only recognizes YAML for application/x-yaml or text/yaml, but the store’s preview logic also allows application/yaml. Add application/yaml here (and consider text/x-yaml) so YAML previews consistently get YAML mode instead of falling back to JSON highlighting.
| const handleDownload = useCallback(() => { | ||
| downloadArtifactFile(artifact.id, artifact.path.split('/').pop() ?? artifact.id) | ||
| }, [artifact.id, artifact.path]) |
There was a problem hiding this comment.
Same empty-filename issue as in ArtifactMetadata: artifact.path.split('/').pop() ?? artifact.id will produce an empty download name if the path ends with /. Prefer || or a non-empty-string check for the fallback.
| http.get('/api/v1/projects/:id', ({ params }) => { | ||
| const project = mockProjects.find((p) => p.id === params.id) | ||
| if (!project) { | ||
| return HttpResponse.json(apiSuccess(null), { status: 404 }) | ||
| } | ||
| return HttpResponse.json(apiSuccess(project)) |
There was a problem hiding this comment.
On 404 this handler returns apiSuccess(null) (success=true, data=null). The client unwrap() treats data: null as an error but will surface an unhelpful "Unknown API error" message because error is null. Return apiError('Project not found') (like the artifacts handler) so Storybook and tests get a meaningful error string.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
web/src/stores/projects.ts (1)
63-63:⚠️ Potential issue | 🟠 MajorRemove fixed fetch caps that silently truncate visible data.
Line 63 (
limit: 200) and Line 78 (limit: 50) can make list/detail views appear complete while omitting projects/tasks. With local filtering and no load-more in this store, users can’t discover truncated items.Also applies to: 78-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/stores/projects.ts` at line 63, The store is hard-capping fetches (e.g., listProjects({ limit: 200 }) and listProjects({ limit: 50 })) which silently truncates visible data; update the calls to listProjects used in this store (e.g., in the load/list and detail-loading functions) to remove fixed limits and instead implement proper pagination or full retrieval: either accept a configurable limit/offset param passed into the store, or loop using the API cursor/next-token until all items are fetched (or document a clear default and expose a "load more" API). Replace the literal limit: 200 and limit: 50 usages with this pagination logic or a parameterized call so no items are silently omitted.web/src/stores/artifacts.ts (2)
117-122:⚠️ Potential issue | 🟡 MinorClear the detail error when deleting the selected artifact.
If the current artifact already had a preview warning in
detailError, Lines 120-121 null the artifact but leave that message behind. The detail page will then show the stale preview error instead of its default “Artifact not found.” state.♻️ Suggested cleanup
set((state) => ({ artifacts: state.artifacts.filter((a) => a.id !== id), totalArtifacts: Math.max(0, state.totalArtifacts - 1), selectedArtifact: state.selectedArtifact?.id === id ? null : state.selectedArtifact, contentPreview: state.selectedArtifact?.id === id ? null : state.contentPreview, + detailLoading: state.selectedArtifact?.id === id ? false : state.detailLoading, + detailError: state.selectedArtifact?.id === id ? null : state.detailError, }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/stores/artifacts.ts` around lines 117 - 122, When removing an artifact in the set updater, also clear any stale detail error for the deleted selected item: if state.selectedArtifact?.id === id then set selectedArtifact to null, contentPreview to null, and detailError to null so the detail page doesn't show the old preview warning; update the set call handling artifacts/totalArtifacts to include detailError: state.selectedArtifact?.id === id ? null : state.detailError (referencing set, artifacts, totalArtifacts, selectedArtifact, contentPreview, and detailError).
73-77:⚠️ Potential issue | 🟠 MajorStop refreshing an unfiltered 200-row snapshot.
fetchArtifacts()still ignores every filter in the store and always requestslistArtifacts({ limit: 200 }). Once the workspace has more than 200 artifacts, the dashboard can only search/filter/manage the first page whiletotalArtifactsstill reports the larger total.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/stores/artifacts.ts` around lines 73 - 77, fetchArtifacts currently ignores the store's filtering/pagination and always calls listArtifacts({ limit: 200 }), causing the UI to only show the first snapshot; update fetchArtifacts to read the store's current filters/pagination (e.g., filter criteria, page/offset, limit) and pass them into listArtifacts (for example listArtifacts({ limit, offset, ...filters })) before setting artifacts and totalArtifacts so results match the active filters; ensure you reference the store's getters (used in this file) to obtain those values and still set listLoading/listError appropriately in the try/catch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/stores/artifacts.ts`:
- Around line 51-53: fetchArtifacts and updateFromWsEvent can race: ensure each
async list-refresh path uses a request token snapshot and skips applying results
if stale. Increment _detailRequestToken when starting a new list fetch, capture
it in a local variable, and call isStaleRequest(token) before mutating state
(including listLoading, listError, and the artifacts list) so an older response
cannot overwrite a newer snapshot; apply the same token-checked guard in
updateFromWsEvent and any other list-update code paths referenced around lines
73-80.
- Around line 87-107: The selectedArtifact and loading state are currently set
only after getArtifactContentText completes, which blocks metadata rendering;
change the sequencing in the function that calls getArtifact(id),
isPreviewableText(), getArtifactContentText(id), and set(...) so that after
awaiting getArtifact(id) you immediately call set({ selectedArtifact: artifact,
detailLoading: false }) (and set contentPreview:null/detailError as appropriate)
to render metadata, then kick off the preview fetch asynchronously (e.g., start
getArtifactContentText in a detached async block or Promise) that still checks
isStaleRequest(token) before applying results; when the preview resolves or
errors, update contentPreview and detailError via set(...) and ensure
partialErrors use getErrorMessage(err) and stale-request checks (refer to
getArtifact, getArtifactContentText, isPreviewableText, selectedArtifact,
contentPreview, detailLoading, detailError, set).
In `@web/src/stores/projects.ts`:
- Around line 107-112: The patch recomputes totalProjects from the local slice
(filtered.length + 1) which can undercount when the store holds a partial
dataset; inside the set((state) => { ... }) updater, preserve and increment the
existing state.totalProjects instead of deriving it from the filtered array—e.g.
set totalProjects to (state.totalProjects ?? filtered.length) + 1 to safely
fallback when totalProjects is missing; update the set callback that manipulates
projects and totalProjects accordingly.
---
Duplicate comments:
In `@web/src/stores/artifacts.ts`:
- Around line 117-122: When removing an artifact in the set updater, also clear
any stale detail error for the deleted selected item: if
state.selectedArtifact?.id === id then set selectedArtifact to null,
contentPreview to null, and detailError to null so the detail page doesn't show
the old preview warning; update the set call handling artifacts/totalArtifacts
to include detailError: state.selectedArtifact?.id === id ? null :
state.detailError (referencing set, artifacts, totalArtifacts, selectedArtifact,
contentPreview, and detailError).
- Around line 73-77: fetchArtifacts currently ignores the store's
filtering/pagination and always calls listArtifacts({ limit: 200 }), causing the
UI to only show the first snapshot; update fetchArtifacts to read the store's
current filters/pagination (e.g., filter criteria, page/offset, limit) and pass
them into listArtifacts (for example listArtifacts({ limit, offset, ...filters
})) before setting artifacts and totalArtifacts so results match the active
filters; ensure you reference the store's getters (used in this file) to obtain
those values and still set listLoading/listError appropriately in the try/catch.
In `@web/src/stores/projects.ts`:
- Line 63: The store is hard-capping fetches (e.g., listProjects({ limit: 200 })
and listProjects({ limit: 50 })) which silently truncates visible data; update
the calls to listProjects used in this store (e.g., in the load/list and
detail-loading functions) to remove fixed limits and instead implement proper
pagination or full retrieval: either accept a configurable limit/offset param
passed into the store, or loop using the API cursor/next-token until all items
are fetched (or document a clear default and expose a "load more" API). Replace
the literal limit: 200 and limit: 50 usages with this pagination logic or a
parameterized call so no items are silently omitted.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4c1d5f30-c52d-495a-9922-93b4d4e5d6fb
📒 Files selected for processing (2)
web/src/stores/artifacts.tsweb/src/stores/projects.ts
📜 Review details
⏰ 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). (8)
- GitHub Check: Agent
- GitHub Check: Dashboard Test
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Web
- GitHub Check: Build Backend
- GitHub Check: Build Sandbox
- GitHub Check: Dependency Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (1)
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx}: Always reuse existing components fromweb/src/components/ui/before creating new ones, including StatusBadge, MetricCard, Sparkline, SectionCard, AgentCard, DeptHealthBar, ProgressGauge, StatPill, Avatar, Button, Toast/ToastContainer, Skeleton variants, EmptyState, ErrorBoundary, ConfirmDialog, CommandPalette, InlineEdit, AnimatedPresence, StaggerGroup/StaggerItem, Drawer, InputField, SelectField, SliderField, ToggleField, TaskStatusIndicator, PriorityBadge, ProviderHealthBadge, TokenUsageBar, CodeMirrorEditor, SegmentedControl, ThemeToggle, LiveRegion, MobileUnsupportedOverlay, LazyCodeMirrorEditor, and TagInput
Use Tailwind semantic classes (text-foreground, bg-card, text-accent, text-success, bg-danger, etc.) or CSS variables (var(--so-accent)) for colors; NEVER hardcode hex values in .tsx/.ts files
Use font-sans or font-mono (maps to Geist tokens) for typography; NEVER set fontFamily directly
Use density-aware tokens (p-card, gap-section-gap, gap-grid-gap) or standard Tailwind spacing; NEVER hardcode pixel values for layout spacing
Use token variables (var(--so-shadow-card-hover), border-border, border-bright) for shadows and borders; NEVER hardcode values
Do NOT recreate status dots inline -- use
Do NOT build card-with-header layouts from scratch -- use
Do NOT create metric displays with text-metric font-bold -- use
Do NOT render initials circles manually -- use
Do NOT create complex (>8 line) JSX inside .map() -- extract to a shared component
Do NOT use rgba() with hardcoded values -- use design token variables
Do NOT use hardcoded Framer Motion transition durations -- use presets from@/lib/motion
web/src/**/*.{ts,tsx}: Apply eslint-web (zero warnings) via pre-commit hooks for the web dashboard (conditional onweb/src/**/*.{ts,tsx})
Always reuse existing components fromweb/src/components/ui/before creating new ones in the React dashboard
Never hardcode hex colors, font-famil...
Files:
web/src/stores/projects.tsweb/src/stores/artifacts.ts
🧠 Learnings (5)
📚 Learning: 2026-03-15T21:49:53.264Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:49:53.264Z
Learning: Fix everything valid — never skip when review agents find valid issues (including pre-existing issues in surrounding code, suggestions, and findings adjacent to the PR's changes). No deferring, no 'out of scope' skipping.
Applied to files:
web/src/stores/projects.ts
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Fix all valid issues found by review agents (including pre-existing issues in surrounding code, suggestions, and adjacent findings) — never skip or defer
Applied to files:
web/src/stores/projects.ts
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Applies to {**/*.py,web/src/**/*.{ts,tsx}} : Keep functions under 50 lines and files under 800 lines
Applied to files:
web/src/stores/projects.ts
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Applies to {**/*.py,web/src/**/*.{ts,tsx}} : Line length: 88 characters (enforced by ruff)
Applied to files:
web/src/stores/projects.ts
📚 Learning: 2026-03-31T14:41:06.345Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2026-03-31T14:41:06.345Z
Learning: Applies to web/src/**/*.stories.tsx : Use storybook/actions instead of storybook/addon-actions for Storybook 10 imports
Applied to files:
web/src/stores/artifacts.ts
🔇 Additional comments (1)
web/src/stores/projects.ts (1)
36-43: Good stale-response protection for concurrent async loads.The request-token guards for list/detail fetches and token invalidation in
clearDetailcorrectly prevent out-of-order responses from clobbering newer state.Also applies to: 59-69, 72-82, 122-130
web/src/stores/artifacts.ts
Outdated
| let _detailRequestToken = 0 | ||
| /** True when a newer detail request has superseded this one. */ | ||
| function isStaleRequest(token: number): boolean { return _detailRequestToken !== token } |
There was a problem hiding this comment.
Guard the list refresh path against stale responses.
fetchArtifacts() can run from polling and updateFromWsEvent() concurrently. If the older request resolves last, it overwrites the newer snapshot and can also drop listLoading/listError for the wrong request.
🛡️ Suggested stale-response guard
+let _listRequestToken = 0
let _detailRequestToken = 0
/** True when a newer detail request has superseded this one. */
function isStaleRequest(token: number): boolean { return _detailRequestToken !== token }
fetchArtifacts: async () => {
+ const token = ++_listRequestToken
set({ listLoading: true, listError: null })
try {
const result = await listArtifacts({ limit: 200 })
+ if (token !== _listRequestToken) return
set({ artifacts: result.data, totalArtifacts: result.total, listLoading: false })
} catch (err) {
+ if (token !== _listRequestToken) return
set({ listLoading: false, listError: getErrorMessage(err) })
}
},Also applies to: 73-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/stores/artifacts.ts` around lines 51 - 53, fetchArtifacts and
updateFromWsEvent can race: ensure each async list-refresh path uses a request
token snapshot and skips applying results if stale. Increment
_detailRequestToken when starting a new list fetch, capture it in a local
variable, and call isStaleRequest(token) before mutating state (including
listLoading, listError, and the artifacts list) so an older response cannot
overwrite a newer snapshot; apply the same token-checked guard in
updateFromWsEvent and any other list-update code paths referenced around lines
73-80.
| set((state) => { | ||
| const filtered = state.projects.filter((p) => p.id !== project.id) | ||
| return { | ||
| projects: [project, ...filtered], | ||
| totalProjects: filtered.length + 1, | ||
| } |
There was a problem hiding this comment.
totalProjects is recomputed from local slice, which can corrupt counts.
Line 111 uses filtered.length + 1, but filtered is only the currently loaded list. If the store holds a partial dataset, this can drop totalProjects to an incorrect value after create.
Proposed fix
set((state) => {
- const filtered = state.projects.filter((p) => p.id !== project.id)
- return {
- projects: [project, ...filtered],
- totalProjects: filtered.length + 1,
- }
+ const exists = state.projects.some((p) => p.id === project.id)
+ if (exists) {
+ return { projects: state.projects }
+ }
+ return {
+ projects: [project, ...state.projects],
+ totalProjects: state.totalProjects + 1,
+ }
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set((state) => { | |
| const filtered = state.projects.filter((p) => p.id !== project.id) | |
| return { | |
| projects: [project, ...filtered], | |
| totalProjects: filtered.length + 1, | |
| } | |
| set((state) => { | |
| const exists = state.projects.some((p) => p.id === project.id) | |
| if (exists) { | |
| return { projects: state.projects } | |
| } | |
| return { | |
| projects: [project, ...state.projects], | |
| totalProjects: state.totalProjects + 1, | |
| } | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/stores/projects.ts` around lines 107 - 112, The patch recomputes
totalProjects from the local slice (filtered.length + 1) which can undercount
when the store holds a partial dataset; inside the set((state) => { ... })
updater, preserve and increment the existing state.totalProjects instead of
deriving it from the filtered array—e.g. set totalProjects to
(state.totalProjects ?? filtered.length) + 1 to safely fallback when
totalProjects is missing; update the set callback that manipulates projects and
totalProjects accordingly.
- Add list request token guard to artifacts store fetchArtifacts to prevent concurrent polling/WS responses from overwriting newer data - Render artifact metadata immediately after getArtifact resolves, then fetch content preview asynchronously in the background so the detail page is not blocked by preview loading - Clear detailError alongside selectedArtifact/contentPreview when deleting the currently viewed artifact - Fix projects MSW handler 404 to use apiError instead of apiSuccess - Add project_id to ArtifactMetadata grid display - Add application/yaml to getLanguage() for consistent YAML syntax highlighting (matches isPreviewableText) - Fix empty filename fallback: use || instead of ?? so paths ending with / fall back to artifact.id - Fix createProject totalProjects: preserve server-reported total and only increment when project ID was not already present Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
web/src/stores/projects.ts (1)
63-63:⚠️ Potential issue | 🟠 MajorRemove fixed fetch caps that silently truncate projects/tasks.
Line 63 (
limit: 200) and Line 78 (limit: 50) can hide valid data while the UI filters and detail view read from local store state. This makes larger workspaces partially unsearchable and project detail tasks potentially incomplete with no load-more path.Please switch to explicit pagination/incremental loading (or full paginated accumulation) instead of hard caps.
Also applies to: 78-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/stores/projects.ts` at line 63, The call using a hard cap (listProjects({ limit: 200 }) and the similar listProjects({ limit: 50 })) truncates results; replace these fixed limits with proper pagination: update the code that calls listProjects to either (A) perform incremental accumulation by repeatedly calling listProjects with the API's cursor/offset until no more pages and merge into the local store, or (B) expose a loadMore pagination API/state so the UI requests additional pages on demand; locate the occurrences of listProjects(...) in this file (and any helper like fetchProjects/fetchProjectTasks if present) and remove the hard-coded limits, wiring in the cursor/offset params and accumulation or loadMore behavior instead of truncating results.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/pages/artifacts/ArtifactMetadata.tsx`:
- Around line 64-98: The Delete button remains clickable while a deletion is in
progress; update the Delete Button (the Button instance that calls
setDeleteOpen(true)) to reflect the deleting state by adding disabled={deleting}
(and/or aria-disabled={deleting} or a loading prop if available) and keep its
onClick no-op when deleting, so that while deleting is true the button cannot
open the ConfirmDialog again; adjust the Button near the Trash2 icon and ensure
the deleting prop is used consistently with the ConfirmDialog and handleDelete.
In `@web/src/stores/artifacts.ts`:
- Around line 148-152: The WebSocket handler updateFromWsEvent is calling
fetchArtifacts() fire-and-forget so network failures are swallowed; make
updateFromWsEvent async, await useArtifactsStore.getState().fetchArtifacts(),
and wrap the await in a try/catch that handles errors (e.g., set the store's
listError or call the app's error/notification helper and console.error) so a
failed refetch updates state or surfaces a notification instead of silently
leaving stale data.
- Around line 77-88: fetchArtifacts currently calls listArtifacts({ limit: 200
}) and ignores store filter state, causing only the first 200 artifacts to be
fetched; update fetchArtifacts to read the active filter fields from the store
(e.g., type, created_by, task_id, content_type, project_id) and pass them into
listArtifacts alongside limit (or implement paginated requests that include
these filters) so the back-end applies filtering correctly; ensure you still use
_listRequestToken and isStaleListRequest as before and set
listLoading/listError/artifacts/totalArtifacts from the API response.
---
Duplicate comments:
In `@web/src/stores/projects.ts`:
- Line 63: The call using a hard cap (listProjects({ limit: 200 }) and the
similar listProjects({ limit: 50 })) truncates results; replace these fixed
limits with proper pagination: update the code that calls listProjects to either
(A) perform incremental accumulation by repeatedly calling listProjects with the
API's cursor/offset until no more pages and merge into the local store, or (B)
expose a loadMore pagination API/state so the UI requests additional pages on
demand; locate the occurrences of listProjects(...) in this file (and any helper
like fetchProjects/fetchProjectTasks if present) and remove the hard-coded
limits, wiring in the cursor/offset params and accumulation or loadMore behavior
instead of truncating results.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a419ade-5ee9-48dd-a8c3-b80e812e76a5
📒 Files selected for processing (5)
web/src/mocks/handlers/projects.tsweb/src/pages/artifacts/ArtifactContentPreview.tsxweb/src/pages/artifacts/ArtifactMetadata.tsxweb/src/stores/artifacts.tsweb/src/stores/projects.ts
📜 Review details
⏰ 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). (6)
- GitHub Check: Build Web
- GitHub Check: Build Backend
- GitHub Check: Dashboard Test
- GitHub Check: Test (Python 3.14)
- GitHub Check: Dependency Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (1)
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx}: Always reuse existing components fromweb/src/components/ui/before creating new ones, including StatusBadge, MetricCard, Sparkline, SectionCard, AgentCard, DeptHealthBar, ProgressGauge, StatPill, Avatar, Button, Toast/ToastContainer, Skeleton variants, EmptyState, ErrorBoundary, ConfirmDialog, CommandPalette, InlineEdit, AnimatedPresence, StaggerGroup/StaggerItem, Drawer, InputField, SelectField, SliderField, ToggleField, TaskStatusIndicator, PriorityBadge, ProviderHealthBadge, TokenUsageBar, CodeMirrorEditor, SegmentedControl, ThemeToggle, LiveRegion, MobileUnsupportedOverlay, LazyCodeMirrorEditor, and TagInput
Use Tailwind semantic classes (text-foreground, bg-card, text-accent, text-success, bg-danger, etc.) or CSS variables (var(--so-accent)) for colors; NEVER hardcode hex values in .tsx/.ts files
Use font-sans or font-mono (maps to Geist tokens) for typography; NEVER set fontFamily directly
Use density-aware tokens (p-card, gap-section-gap, gap-grid-gap) or standard Tailwind spacing; NEVER hardcode pixel values for layout spacing
Use token variables (var(--so-shadow-card-hover), border-border, border-bright) for shadows and borders; NEVER hardcode values
Do NOT recreate status dots inline -- use
Do NOT build card-with-header layouts from scratch -- use
Do NOT create metric displays with text-metric font-bold -- use
Do NOT render initials circles manually -- use
Do NOT create complex (>8 line) JSX inside .map() -- extract to a shared component
Do NOT use rgba() with hardcoded values -- use design token variables
Do NOT use hardcoded Framer Motion transition durations -- use presets from@/lib/motion
web/src/**/*.{ts,tsx}: Apply eslint-web (zero warnings) via pre-commit hooks for the web dashboard (conditional onweb/src/**/*.{ts,tsx})
Always reuse existing components fromweb/src/components/ui/before creating new ones in the React dashboard
Never hardcode hex colors, font-famil...
Files:
web/src/mocks/handlers/projects.tsweb/src/pages/artifacts/ArtifactContentPreview.tsxweb/src/pages/artifacts/ArtifactMetadata.tsxweb/src/stores/projects.tsweb/src/stores/artifacts.ts
🧠 Learnings (5)
📚 Learning: 2026-03-31T14:41:06.345Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2026-03-31T14:41:06.345Z
Learning: Applies to web/src/**/*.{ts,tsx} : Always reuse existing components from `web/src/components/ui/` before creating new ones, including StatusBadge, MetricCard, Sparkline, SectionCard, AgentCard, DeptHealthBar, ProgressGauge, StatPill, Avatar, Button, Toast/ToastContainer, Skeleton variants, EmptyState, ErrorBoundary, ConfirmDialog, CommandPalette, InlineEdit, AnimatedPresence, StaggerGroup/StaggerItem, Drawer, InputField, SelectField, SliderField, ToggleField, TaskStatusIndicator, PriorityBadge, ProviderHealthBadge, TokenUsageBar, CodeMirrorEditor, SegmentedControl, ThemeToggle, LiveRegion, MobileUnsupportedOverlay, LazyCodeMirrorEditor, and TagInput
Applied to files:
web/src/pages/artifacts/ArtifactMetadata.tsx
📚 Learning: 2026-03-15T21:49:53.264Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:49:53.264Z
Learning: Fix everything valid — never skip when review agents find valid issues (including pre-existing issues in surrounding code, suggestions, and findings adjacent to the PR's changes). No deferring, no 'out of scope' skipping.
Applied to files:
web/src/stores/projects.ts
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Fix all valid issues found by review agents (including pre-existing issues in surrounding code, suggestions, and adjacent findings) — never skip or defer
Applied to files:
web/src/stores/projects.ts
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Applies to {**/*.py,web/src/**/*.{ts,tsx}} : Keep functions under 50 lines and files under 800 lines
Applied to files:
web/src/stores/projects.ts
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Applies to {**/*.py,web/src/**/*.{ts,tsx}} : Line length: 88 characters (enforced by ruff)
Applied to files:
web/src/stores/projects.ts
🔇 Additional comments (10)
web/src/mocks/handlers/projects.ts (1)
1-54: LGTM!The MSW handlers are well-structured and follow established patterns:
- Mock data correctly implements the
Projectinterface with good variety (nullable fields tested).- Pagination metadata dynamically derives
totalfrom mock data length.- Both handlers use the
apiSuccess/apiErrorhelpers for consistent API envelope.- The detail handler correctly returns 404 with proper status code for missing projects.
- Type safety is maintained throughout (string ID comparison is correct per
Project.idtype).web/src/stores/projects.ts (1)
60-68: Stale-response protection is well implemented here.The monotonic request tokens plus
clearDetailinvalidation correctly prevent older async responses from overwriting newer state.Also applies to: 73-81, 123-130
web/src/pages/artifacts/ArtifactContentPreview.tsx (3)
17-24:getLanguagefallback to'json'for non-JSON/YAML text types may cause incorrect highlighting.This was noted in a prior review but remains unaddressed. For
text/plain,text/markdown,text/html, etc., the function returns'json', which applies JSON syntax highlighting to non-JSON content. Consider adding more content-type mappings or a neutral fallback if CodeMirror supports one.
1-39: LGTM — Component structure and hook usage are correct.The component properly:
- Uses
useCallbackforhandleDownloadbefore early returns (addressing prior feedback)- Manages blob URL lifecycle with proper cleanup in
useEffect- Correctly excludes SVG to prevent XSS risks
- Reuses existing UI components (
SectionCard,Skeleton,EmptyState,LazyCodeMirrorEditor)
42-65: Blob URL lifecycle management is well-implemented.The
useEffectcorrectly:
- Sets
revokedflag to prevent state updates after unmount- Resets
imageSrcandimageErroron cleanup- Revokes the object URL via
imageSrcRefto prevent memory leaks- Dependencies array includes all relevant values
web/src/stores/artifacts.ts (3)
51-57: Stale-response protection is well-implemented.The monotonic request token pattern correctly handles race conditions for both list and detail fetches. Each async operation captures a token at start and checks
isStaleDetailRequest/isStaleListRequestbefore applying state updates.
90-115: Detail fetch correctly renders metadata before preview loads.The implementation:
- Clears stale state at start (line 92)
- Sets
selectedArtifactanddetailLoading: falseimmediately after metadata arrives (line 98)- Fetches content preview in background with proper stale-request guards
- Partial failures set
detailErrorwhile keeping metadata visibleThis addresses prior feedback about blocking the detail page on preview downloads.
117-129: Delete correctly clears selected artifact state when deleting the viewed artifact.The implementation checks
isSelectedand conditionally clearsselectedArtifact,contentPreview, anddetailError. This addresses prior feedback about stale detail data after deletion.web/src/pages/artifacts/ArtifactMetadata.tsx (2)
21-44: LGTM — Component follows design system and coding guidelines.The implementation correctly:
- Reuses existing UI components (
SectionCard,MetadataGrid,ContentTypeBadge,Button,ConfirmDialog)- Uses semantic Tailwind classes (
text-danger,hover:bg-danger/10,text-muted-foreground,font-mono)- Properly formats metadata values with appropriate utilities
Based on learnings: "Always reuse existing components from
web/src/components/ui/before creating new ones."
46-62: Delete handler correctly manages async flow and error states.The implementation:
- Sets
deleting: truebefore async operation- Properly catches errors and shows toast with
getErrorMessage- Uses
finallyto reset state regardless of outcome- Navigates away only on success
| fetchArtifacts: async () => { | ||
| const token = ++_listRequestToken | ||
| set({ listLoading: true, listError: null }) | ||
| try { | ||
| const result = await listArtifacts({ limit: 200 }) | ||
| if (isStaleListRequest(token)) return | ||
| set({ artifacts: result.data, totalArtifacts: result.total, listLoading: false }) | ||
| } catch (err) { | ||
| if (isStaleListRequest(token)) return | ||
| set({ listLoading: false, listError: getErrorMessage(err) }) | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the listArtifacts API endpoint supports filter parameters
rg -n "listArtifacts" --type=ts -C3Repository: Aureliolo/synthorg
Length of output: 4750
🏁 Script executed:
# Check ArtifactFilters type definition
rg -n "ArtifactFilters" --type=ts -A5Repository: Aureliolo/synthorg
Length of output: 3182
🏁 Script executed:
# Check the full store implementation for filter state
cat -n web/src/stores/artifacts.ts | head -100Repository: Aureliolo/synthorg
Length of output: 4092
🏁 Script executed:
# Check how filters are managed in the UI or other parts
rg -n "searchQuery|typeFilter|createdByFilter" web/src --type=ts -B2 -A2Repository: Aureliolo/synthorg
Length of output: 26914
🏁 Script executed:
# Check the full useArtifactsData implementation to understand filtering flow
cat -n web/src/hooks/useArtifactsData.tsRepository: Aureliolo/synthorg
Length of output: 4180
🏁 Script executed:
# Check if there's pagination logic elsewhere or if offset is used
rg -n "offset|pagination" web/src/stores/artifacts.ts web/src/hooks/useArtifactsData.ts --type=tsRepository: Aureliolo/synthorg
Length of output: 44
Store does not forward active filters to the backend API — only first 200 artifacts are fetched.
fetchArtifacts calls listArtifacts({ limit: 200 }) without passing filter parameters (type, created_by, task_id, content_type, project_id). Although the API supports these filters, they are applied only client-side in useArtifactsData after fetch, limiting search results to whatever is in the 200-artifact set. Users cannot access matching artifacts beyond position 200 even though totalArtifacts indicates they exist on the backend.
Modify fetchArtifacts to forward active store filters to the API call, or implement pagination with filters if backend constraints require it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/stores/artifacts.ts` around lines 77 - 88, fetchArtifacts currently
calls listArtifacts({ limit: 200 }) and ignores store filter state, causing only
the first 200 artifacts to be fetched; update fetchArtifacts to read the active
filter fields from the store (e.g., type, created_by, task_id, content_type,
project_id) and pass them into listArtifacts alongside limit (or implement
paginated requests that include these filters) so the back-end applies filtering
correctly; ensure you still use _listRequestToken and isStaleListRequest as
before and set listLoading/listError/artifacts/totalArtifacts from the API
response.
| // Event payload ignored -- all events trigger a full refetch. | ||
| // Incremental updates are not worth the complexity given 30s polling. | ||
| updateFromWsEvent: () => { | ||
| useArtifactsStore.getState().fetchArtifacts() | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider awaiting fetchArtifacts() or handling errors in WS event handler.
updateFromWsEvent calls fetchArtifacts() without awaiting it or catching errors. While this is fire-and-forget by design (per the comment), any fetch failure will silently set listError without notification. If the WS event indicates new data but the refetch fails, the user sees stale data with no indication of the problem.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/stores/artifacts.ts` around lines 148 - 152, The WebSocket handler
updateFromWsEvent is calling fetchArtifacts() fire-and-forget so network
failures are swallowed; make updateFromWsEvent async, await
useArtifactsStore.getState().fetchArtifacts(), and wrap the await in a try/catch
that handles errors (e.g., set the store's listError or call the app's
error/notification helper and console.error) so a failed refetch updates state
or surfaces a notification instead of silently leaving stale data.
Prevents reopening the ConfirmDialog while a delete operation
is in progress by adding disabled={deleting} to the trigger button.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
🤖 I have created a release *beep* *boop* --- ## [0.5.4](v0.5.3...v0.5.4) (2026-04-01) ### Features * artifact and project management UI in web dashboard ([#954](#954)) ([00a0430](00a0430)) * embed MkDocs build output in React web dashboard at /docs ([#948](#948)) ([f229fc2](f229fc2)) * personality preset discovery API and user-defined preset CRUD ([#952](#952)) ([497848a](497848a)) * support multi-provider model resolution with budget-based selection ([#953](#953)) ([146b782](146b782)) * support per-agent memory retention overrides ([#209](#209)) ([#951](#951)) ([020c610](020c610)) ### Documentation * write user guides and tutorials ([#949](#949)) ([1367225](1367225)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
Test plan
uv run ruff check src/ tests/-- all passeduv run mypy src/ tests/-- all passednpm --prefix web run type-check-- all passednpm --prefix web run lint-- zero warningsnpm --prefix web run test-- 194 files, 2326 tests passednpm --prefix web run build-- production build succeedsPre-reviewed by 8 agents (code-reviewer, frontend-reviewer, api-contract-drift, silent-failure-hunter, issue-resolution-verifier, design-token-audit, test-quality-reviewer, docs-consistency), 17 findings addressed.
Closes #946
🤖 Generated with Claude Code