The design agent is now drafting the full implementation plan — cover…#40
Conversation
…ing the AttachmentManager, DB migration, IPC surface, agent injection (manifest + `additionalDirectories` + image content blocks), composer UI with the circular progress ring, and the settings section. I'll review its output against the code and write the final plan when it reports back.
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR implements a comprehensive attachment system for file uploads with strong security controls. The architecture properly isolates the renderer from filesystem access and implements defense-in-depth validation.
Security strengths:
- Symlink resolution with
fs.realpathSync - Path traversal protection blocking userData directory access
- Magic byte validation for images
- SHA-256 content deduplication
- SQL prepared statements throughout
- Atomic writes with temp+rename pattern
Critical finding:
1 security vulnerability requiring fix before merge - null byte validation missing in IPC handler could allow path truncation attacks.
The implementation is well-documented and follows security best practices overall. Once the null byte validation is added, this will be ready to merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| for (const p of paths) { | ||
| if (typeof p !== 'string' || p.length === 0 || p.length > ATTACHMENT_LIMITS.pathMax) { | ||
| throw new Error('Invalid path'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛑 Security Vulnerability: Missing null byte validation on paths. The attachmentAddPaths handler validates path length but doesn't check for null bytes (\0), while AttachmentManager.stageOne does check for this (line 506). An attacker could bypass the AttachmentManager's null byte check if paths containing \0 pass through IPC validation, potentially causing path truncation attacks.1
| for (const p of paths) { | |
| if (typeof p !== 'string' || p.length === 0 || p.length > ATTACHMENT_LIMITS.pathMax) { | |
| throw new Error('Invalid path'); | |
| } | |
| } | |
| for (const p of paths) { | |
| if (typeof p !== 'string' || p.length === 0 || p.length > ATTACHMENT_LIMITS.pathMax || p.includes('\0')) { | |
| throw new Error('Invalid path'); | |
| } | |
| } |
Footnotes
-
CWE-158: Improper Neutralization of Null Byte or NUL Character - https://cwe.mitre.org/data/definitions/158.html ↩
📝 WalkthroughWalkthroughThis PR adds session-scoped attachments, updates voice warm/start behavior, and rewires build/release scripts and publish settings. It also updates shared contracts, IPC, renderer state/UI, and documentation to match those features. ChangesAttachment Manager
Voice Startup
Release and packaging
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Composer
participant useAttachmentStore
participant AttachmentManager
participant Database
Composer->>useAttachmentStore: pickFiles / addDropped / pasteImage
useAttachmentStore->>AttachmentManager: addFromPaths / addFromBytes
AttachmentManager->>AttachmentManager: validate, hash, dedup, stage
AttachmentManager->>Database: insert attachment row
AttachmentManager-->>useAttachmentStore: attachmentsChanged / attachmentProgress
sequenceDiagram
participant Renderer
participant VoiceHandlers
participant VoiceManager
participant Worker
Renderer->>VoiceHandlers: voiceWarm
VoiceHandlers->>VoiceManager: warm()
Renderer->>VoiceManager: startCapture
VoiceManager->>Worker: ensureWorker / load VAD / load STT
Suggested labels: feature, main-process, renderer, release, voice Suggested reviewers: Maintainers familiar with attachment flow, IPC boundaries, voice capture, and build/release scripts 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/reference/window-limboo-api.md`:
- Around line 13-19: The fenced namespace list in the documentation lacks a
language tag, which triggers the markdownlint MD040 warning. Update the code
fence in the window.limboo namespace list to use a plain-text tag such as text
or plaintext so the rendered output stays the same and the lint warning is
silenced.
In `@src/main/managers/attachments/AttachmentManager.ts`:
- Around line 551-565: The binary sniff in AttachmentManager’s validation path
is using the small image head buffer, so `looksBinary()` only sees 16 bytes and
can miss NULs later in the file. Update the `AttachmentManager` flow to read a
larger sniff window for the `code`/`document`/`data` binary check (using the
existing `FS_LIMITS.binarySniffBytes`-sized logic), while keeping the 16-byte
buffer only for `imageMagicMatches` and the image MIME validation.
- Around line 367-406: The imageBlocksFor method currently enforces only
per-image limits, so the combined vision payload can still exceed the request
budget. Update AttachmentManager.imageBlocksFor to track a running total across
all returned image blocks, using ATTACHMENT_LIMITS.imageVisionMaxBytes and the
Messages API cap as the overall budget, and stop inlining additional images once
the aggregate would overflow. Keep oversize entries on the manifest-only path by
skipping them in the same loop that handles nativeImage resizing and base64
encoding.
In `@src/renderer/features/workspace/AttachmentChip.tsx`:
- Around line 42-50: The clickable container in AttachmentChip is missing
keyboard accessibility when onClick is provided. Update AttachmentChip so the
wrapping div behaves like an accessible button by adding an appropriate role,
tabIndex, and keyboard handler in addition to onClick, and ensure Enter/Space
trigger the same action. Keep the logic scoped to the AttachmentChip component
so all consumers like AttachmentStrip and MessageAttachments inherit the fix.
In `@src/renderer/features/workspace/Composer.tsx`:
- Around line 196-214: The dropzone in Composer is clearing drag state too
eagerly because onDragLeave fires when the cursor moves over child elements.
Update the dropzone handlers to ignore internal transitions into descendants
(for example by checking whether the related target is still inside the current
dropzone before calling setDragOver(false)), and keep the existing
onDragOver/onDrop behavior intact so the border only resets when the drag truly
leaves the Composer dropzone.
In `@src/renderer/stores/useAttachmentStore.ts`:
- Around line 52-75: The cleanup in useAttachmentStore’s hydrate/onChanged flow
is too broad because it rebuilds the global progress map from only the current
session’s attachments, which can erase progress for unrelated sessions. Update
the onChanged handler to scope removal to stale ids for the specific sessionId
being updated: preserve any existing progress entries not associated with that
session, and only drop progress for attachment ids in that session that are no
longer uploading while still merging the new bySession state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6cb303d9-9bd6-484a-a012-2c749d0eb828
📒 Files selected for processing (26)
CLAUDE.mddocs/reference/window-limboo-api.mdsrc/main/db/database.tssrc/main/index.tssrc/main/ipc/agentHandlers.tssrc/main/ipc/attachmentHandlers.tssrc/main/ipc/index.tssrc/main/ipc/sessionHandlers.tssrc/main/managers/AgentManager.tssrc/main/managers/SettingsManager.tssrc/main/managers/attachments/AttachmentManager.tssrc/main/managers/attachments/validate.tssrc/preload/index.tssrc/renderer/App.tsxsrc/renderer/features/settings/catalog.tsxsrc/renderer/features/settings/panels/AttachmentsPanel.tsxsrc/renderer/features/workspace/AttachmentChip.tsxsrc/renderer/features/workspace/AttachmentStrip.tsxsrc/renderer/features/workspace/CenterWorkspace.tsxsrc/renderer/features/workspace/Composer.tsxsrc/renderer/features/workspace/ConversationView.tsxsrc/renderer/stores/useAgentStore.tssrc/renderer/stores/useAttachmentStore.tssrc/shared/constants.tssrc/shared/ipc-channels.tssrc/shared/types.ts
| The API has 18 namespaces: | ||
|
|
||
| ``` | ||
| window.limboo.{ window, settings, system, app, events, | ||
| workspace, session, agent, fs, terminal, git, | ||
| worktree, services, memory, search, updates, voice } | ||
| worktree, services, memory, search, updates, voice, | ||
| attachment } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the namespace list fence.
This fenced block will trigger the MD040 markdownlint warning. text or plaintext keeps the rendered output unchanged and silences the lint error.
Proposed fix
-```
+```text
window.limboo.{ window, settings, system, app, events,
workspace, session, agent, fs, terminal, git,
worktree, services, memory, search, updates, voice,
attachment }
-```
+```📝 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.
| The API has 18 namespaces: | |
| ``` | |
| window.limboo.{ window, settings, system, app, events, | |
| workspace, session, agent, fs, terminal, git, | |
| worktree, services, memory, search, updates, voice } | |
| worktree, services, memory, search, updates, voice, | |
| attachment } |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/reference/window-limboo-api.md` around lines 13 - 19, The fenced
namespace list in the documentation lacks a language tag, which triggers the
markdownlint MD040 warning. Update the code fence in the window.limboo namespace
list to use a plain-text tag such as text or plaintext so the rendered output
stays the same and the lint warning is silenced.
Source: Linters/SAST tools
| imageBlocksFor(sessionId: string, ids: string[]): AttachmentImageBlock[] { | ||
| const cfg = this.settings.getAll().attachments; | ||
| if (!cfg.images.attachAsVision) return []; | ||
| const dir = this.sessionDir(sessionId); | ||
| const blocks: AttachmentImageBlock[] = []; | ||
| for (const meta of this.byIds(sessionId, ids)) { | ||
| if (meta.category !== 'image' || !VISION_MEDIA_TYPES.has(meta.mime)) continue; | ||
| const file = path.join(dir, meta.storedName); | ||
| let buf: Buffer; | ||
| try { | ||
| buf = fs.readFileSync(file); | ||
| } catch { | ||
| continue; | ||
| } | ||
| let mediaType = meta.mime; | ||
| const threshold = cfg.images.downscaleThresholdMB * 1024 * 1024; | ||
| if (buf.length > threshold) { | ||
| const img = nativeImage.createFromPath(file); | ||
| if (!img.isEmpty()) { | ||
| const { width, height } = img.getSize(); | ||
| const scale = Math.sqrt(threshold / buf.length); | ||
| const resized = img.resize({ | ||
| width: Math.max(64, Math.round(width * scale)), | ||
| height: Math.max(64, Math.round(height * scale)), | ||
| }); | ||
| buf = resized.toJPEG(80); | ||
| mediaType = 'image/jpeg'; | ||
| } | ||
| } | ||
| if (buf.length > ATTACHMENT_LIMITS.imageVisionMaxBytes) { | ||
| logger.warn(`Attachment ${meta.name} exceeds the vision size cap after downscale; manifest-only.`); | ||
| continue; | ||
| } | ||
| blocks.push({ | ||
| type: 'image', | ||
| source: { type: 'base64', media_type: mediaType, data: buf.toString('base64') }, | ||
| }); | ||
| } | ||
| return blocks; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Anthropic Messages API image content block total request size limit and max images per request
💡 Result:
The Anthropic Messages API enforces specific limits on image content and overall request size to maintain performance and reliability [1][2]. Request Size Limits The maximum allowed request size for the Messages API is 32 MB [3][2]. This is a raw byte limit on the entire HTTP request body, which includes all message content, base64-encoded images, system prompts, and tool definitions [3]. Because base64 encoding adds approximately 33% to the file size of binary data, images should be sized and compressed accordingly before being included in a request [3]. Max Images per Request The maximum number of images allowed per API request depends on the specific model context window: - For models with a 200k-token context window, the limit is 100 images per request [1]. - For all other models, the limit is 600 images per request [1]. Note that on claude.ai, the limit is 20 images per message [1]. Additional Image Constraints - Maximum Dimensions: Images must not exceed 8000x8000 pixels [4][1]. If a single request contains more than 20 images, a stricter dimension limit applies; to stay under this limit across all platforms, it is recommended to keep image dimensions below 2000x2000 pixels or limit the request to 20 or fewer images and document blocks [4][1]. - File Size (per image): When using the Claude API directly, the maximum size per image is 10 MB (base64-encoded) [1]. Platforms like Amazon Bedrock and Google Cloud have a lower limit of 5 MB (base64-encoded) [1]. - Handling Large Payloads: For scenarios involving many images, it is recommended to use the Files API to upload content and reference it via a file_id, rather than including raw base64 data, to help keep the overall request payload under the 32 MB limit [1].
Citations:
- 1: https://platform.claude.com/docs/en/build-with-claude/vision?_rsc=1ebj0
- 2: https://claudecode.jp/en/docs/api/errors
- 3: https://claudecodeguides.com/claude-api-error-413-requesttoolarge-explained/
- 4: https://platform.claude.com/docs/en/build-with-claude/vision
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== locate relevant symbols =='
rg -n "imageBlocksFor|maxFilesPerMessage|imageVisionMaxBytes|attachAsVision|downscaleThresholdMB|VISION_MEDIA_TYPES|manifest-only|attachment" src/main -g '*.ts'
echo
echo '== map AttachmentManager =='
ast-grep outline src/main/managers/attachments/AttachmentManager.ts --view expanded
echo
echo '== surrounding lines in AttachmentManager =='
sed -n '320,440p' src/main/managers/attachments/AttachmentManager.ts
echo
echo '== inspect attachment limits =='
fd -a "Attachment" src/main | sed -n '1,50p'
rg -n "ATTACHMENT_LIMITS|imageVisionMaxBytes|maxFilesPerMessage" src/main -g '*.ts'Repository: BotCoder254/limboo
Length of output: 26435
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== shared attachment constants =='
rg -n "imageVisionMaxBytes|maxFilesPerMessage|downscaleThresholdMB|manifestCharBudget" src/shared src/main -g '*.ts' -A 3 -B 3
echo
echo '== AgentManager call site around imageBlocksFor =='
sed -n '1390,1435p' src/main/managers/AgentManager.ts
echo
echo '== attachment settings defaults =='
sed -n '180,250p' src/main/managers/SettingsManager.ts
echo
echo '== any total vision/request size handling =='
rg -n "base64|vision|request size|32 MB|image count|imageBlocksFor|manifestFor" src/main -g '*.ts' -A 2 -B 2Repository: BotCoder254/limboo
Length of output: 35401
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import base64, math
# show overhead for common raw sizes and total for 10 images at 5 MiB raw cap
for raw in [1_000_000, 5 * 1024 * 1024]:
b64 = 4 * math.ceil(raw / 3)
print(raw, b64, b64 / raw)
print("10x 5MiB raw => base64 bytes:", 10 * (4 * math.ceil((5 * 1024 * 1024) / 3)))
PYRepository: BotCoder254/limboo
Length of output: 255
Cap the total vision payload, not just each image. src/main/managers/attachments/AttachmentManager.ts:367-406
With maxFilesPerMessage at 10 and imageVisionMaxBytes at 5 MiB per image, the combined base64 payload can exceed the Messages API’s 32 MB request limit and fail the whole send. Track a running request budget and spill over-budget images back to the manifest path.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 376-376: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(file)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/managers/attachments/AttachmentManager.ts` around lines 367 - 406,
The imageBlocksFor method currently enforces only per-image limits, so the
combined vision payload can still exceed the request budget. Update
AttachmentManager.imageBlocksFor to track a running total across all returned
image blocks, using ATTACHMENT_LIMITS.imageVisionMaxBytes and the Messages API
cap as the overall budget, and stop inlining additional images once the
aggregate would overflow. Keep oversize entries on the manifest-only path by
skipping them in the same loop that handles nativeImage resizing and base64
encoding.
| // Head sniff: image magic consistency + text/binary confusion. | ||
| const mime = mimeFor(name, category); | ||
| const head = Buffer.alloc(Math.min(SNIFF_BYTES, stat.size)); | ||
| const fd = fs.openSync(real, 'r'); | ||
| try { | ||
| fs.readSync(fd, head, 0, head.length, 0); | ||
| } finally { | ||
| fs.closeSync(fd); | ||
| } | ||
| if (category === 'image' && VISION_MEDIA_TYPES.has(mime) && !imageMagicMatches(mime, head)) { | ||
| throw new AttachmentError('Image data does not match its extension.'); | ||
| } | ||
| if ((category === 'code' || category === 'document' || category === 'data') && looksBinary(head)) { | ||
| category = 'other'; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C2 'binarySniffBytes' src/shared/constants.ts
rg -nP -C3 'binarySniffBytes' --type=tsRepository: BotCoder254/limboo
Length of output: 405
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant constants and helper implementation.
rg -n -C 2 'SNIFF_BYTES|binarySniffBytes|looksBinary\(' src/main src/shared
# Read the relevant slices if they exist.
grep -n 'SNIFF_BYTES\|binarySniffBytes\|looksBinary\(' -n src/main/managers/attachments/AttachmentManager.ts src/shared/constants.ts 2>/dev/null || trueRepository: BotCoder254/limboo
Length of output: 3801
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant attachment staging block and the shared helper definitions.
sed -n '500,575p' src/main/managers/attachments/AttachmentManager.ts
printf '\n--- validate.ts ---\n'
sed -n '120,150p' src/main/managers/attachments/validate.ts
printf '\n--- fs/reader.ts ---\n'
sed -n '1,110p' src/main/managers/fs/reader.tsRepository: BotCoder254/limboo
Length of output: 8065
Binary sniff is capped at 16 bytes here. looksBinary() only inspects head.length, so NULs after byte 16 never reclassify code/document/data to other, even though FS_LIMITS.binarySniffBytes is 8,000. Read the larger sniff window for this check and keep only the first 16 bytes for image magic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/managers/attachments/AttachmentManager.ts` around lines 551 - 565,
The binary sniff in AttachmentManager’s validation path is using the small image
head buffer, so `looksBinary()` only sees 16 bytes and can miss NULs later in
the file. Update the `AttachmentManager` flow to read a larger sniff window for
the `code`/`document`/`data` binary check (using the existing
`FS_LIMITS.binarySniffBytes`-sized logic), while keeping the 16-byte buffer only
for `imageMagicMatches` and the image MIME validation.
Source: Linters/SAST tools
| <div | ||
| className={cn( | ||
| 'group flex max-w-[220px] items-center gap-2 rounded-lg border bg-surface px-2 py-1.5', | ||
| warn ? 'border-warning/40' : 'border-line', | ||
| onClick && 'cursor-pointer transition-colors hover:bg-elevated', | ||
| )} | ||
| onClick={onClick} | ||
| title={meta.error ?? `${meta.name} — ${formatBytes(meta.size)}`} | ||
| > |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clickable chip is not keyboard-accessible.
When onClick is provided (reveal-in-Finder for both draft and sent chips), the container div has no role, tabIndex, or onKeyDown, so keyboard-only users can't trigger it. This affects every consumer of AttachmentChip (AttachmentStrip.tsx, ConversationView.tsx's MessageAttachments).
♿ Proposed fix
<div
className={cn(
'group flex max-w-[220px] items-center gap-2 rounded-lg border bg-surface px-2 py-1.5',
warn ? 'border-warning/40' : 'border-line',
onClick && 'cursor-pointer transition-colors hover:bg-elevated',
)}
onClick={onClick}
+ role={onClick ? 'button' : undefined}
+ tabIndex={onClick ? 0 : undefined}
+ onKeyDown={
+ onClick
+ ? (e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ onClick();
+ }
+ }
+ : undefined
+ }
title={meta.error ?? `${meta.name} — ${formatBytes(meta.size)}`}
>📝 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.
| <div | |
| className={cn( | |
| 'group flex max-w-[220px] items-center gap-2 rounded-lg border bg-surface px-2 py-1.5', | |
| warn ? 'border-warning/40' : 'border-line', | |
| onClick && 'cursor-pointer transition-colors hover:bg-elevated', | |
| )} | |
| onClick={onClick} | |
| title={meta.error ?? `${meta.name} — ${formatBytes(meta.size)}`} | |
| > | |
| <div | |
| className={cn( | |
| 'group flex max-w-[220px] items-center gap-2 rounded-lg border bg-surface px-2 py-1.5', | |
| warn ? 'border-warning/40' : 'border-line', | |
| onClick && 'cursor-pointer transition-colors hover:bg-elevated', | |
| )} | |
| onClick={onClick} | |
| role={onClick ? 'button' : undefined} | |
| tabIndex={onClick ? 0 : undefined} | |
| onKeyDown={ | |
| onClick | |
| ? (e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault(); | |
| onClick(); | |
| } | |
| } | |
| : undefined | |
| } | |
| title={meta.error ?? `${meta.name} — ${formatBytes(meta.size)}`} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/features/workspace/AttachmentChip.tsx` around lines 42 - 50, The
clickable container in AttachmentChip is missing keyboard accessibility when
onClick is provided. Update AttachmentChip so the wrapping div behaves like an
accessible button by adding an appropriate role, tabIndex, and keyboard handler
in addition to onClick, and ensure Enter/Space trigger the same action. Keep the
logic scoped to the AttachmentChip component so all consumers like
AttachmentStrip and MessageAttachments inherit the fix.
| <div | ||
| data-dropzone | ||
| onDragOver={(e) => { | ||
| if (!canAttach) return; | ||
| e.preventDefault(); | ||
| e.dataTransfer.dropEffect = 'copy'; | ||
| setDragOver(true); | ||
| }} | ||
| onDragLeave={() => setDragOver(false)} | ||
| onDrop={onDrop} | ||
| className={cn( | ||
| 'flex flex-col rounded-3xl border bg-surface-2 px-4 py-2.5 shadow-[0_8px_30px_rgba(0,0,0,0.6)] transition-colors focus-within:border-line-strong', | ||
| fileDragActive && canAttach | ||
| ? dragOver | ||
| ? 'border-accent' | ||
| : 'border-line-strong border-dashed' | ||
| : 'border-line', | ||
| )} | ||
| > |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Drag-leave fires on inner-element hover, causing border flicker.
onDragLeave fires whenever the drag crosses into a child element (textarea, buttons, chips) inside the dropzone, briefly clearing dragOver and flickering the border between the drag-active and idle styles.
🎨 Suggested fix — ignore drag-leave into a child
- onDragLeave={() => setDragOver(false)}
+ onDragLeave={(e) => {
+ if (!e.currentTarget.contains(e.relatedTarget as Node | null)) setDragOver(false);
+ }}📝 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.
| <div | |
| data-dropzone | |
| onDragOver={(e) => { | |
| if (!canAttach) return; | |
| e.preventDefault(); | |
| e.dataTransfer.dropEffect = 'copy'; | |
| setDragOver(true); | |
| }} | |
| onDragLeave={() => setDragOver(false)} | |
| onDrop={onDrop} | |
| className={cn( | |
| 'flex flex-col rounded-3xl border bg-surface-2 px-4 py-2.5 shadow-[0_8px_30px_rgba(0,0,0,0.6)] transition-colors focus-within:border-line-strong', | |
| fileDragActive && canAttach | |
| ? dragOver | |
| ? 'border-accent' | |
| : 'border-line-strong border-dashed' | |
| : 'border-line', | |
| )} | |
| > | |
| <div | |
| data-dropzone | |
| onDragOver={(e) => { | |
| if (!canAttach) return; | |
| e.preventDefault(); | |
| e.dataTransfer.dropEffect = 'copy'; | |
| setDragOver(true); | |
| }} | |
| onDragLeave={(e) => { | |
| if (!e.currentTarget.contains(e.relatedTarget as Node | null)) setDragOver(false); | |
| }} | |
| onDrop={onDrop} | |
| className={cn( | |
| 'flex flex-col rounded-3xl border bg-surface-2 px-4 py-2.5 shadow-[0_8px_30px_rgba(0,0,0,0.6)] transition-colors focus-within:border-line-strong', | |
| fileDragActive && canAttach | |
| ? dragOver | |
| ? 'border-accent' | |
| : 'border-line-strong border-dashed' | |
| : 'border-line', | |
| )} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/features/workspace/Composer.tsx` around lines 196 - 214, The
dropzone in Composer is clearing drag state too eagerly because onDragLeave
fires when the cursor moves over child elements. Update the dropzone handlers to
ignore internal transitions into descendants (for example by checking whether
the related target is still inside the current dropzone before calling
setDragOver(false)), and keep the existing onDragOver/onDrop behavior intact so
the border only resets when the drag truly leaves the Composer dropzone.
| hydrate: () => { | ||
| const api = window.limboo?.attachment; | ||
| if (!api || get().hydrated) return; | ||
| api.onChanged(({ sessionId, attachments }) => { | ||
| set((state) => { | ||
| // Drop progress entries for attachments that finished staging. | ||
| const uploading = new Set( | ||
| attachments.filter((a) => a.status === 'uploading').map((a) => a.id), | ||
| ); | ||
| const progress: Record<string, number> = {}; | ||
| for (const [id, pct] of Object.entries(state.progress)) { | ||
| if (uploading.has(id)) progress[id] = pct; | ||
| } | ||
| return { | ||
| bySession: { ...state.bySession, [sessionId]: attachments }, | ||
| progress, | ||
| }; | ||
| }); | ||
| }); | ||
| api.onProgress(({ id, percent }) => { | ||
| set((state) => ({ progress: { ...state.progress, [id]: percent } })); | ||
| }); | ||
| set({ hydrated: true }); | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cross-session progress data gets wiped by unrelated session's attachment:changed event.
progress is a single map keyed by attachment id across all sessions, but the cleanup in onChanged only preserves ids present in the uploading set derived from the current session's attachments payload (line 58-60). Any attachment id belonging to a different session that is still uploading is absent from that set and gets dropped from progress here (lines 61-64), even though it's unrelated to this event. This means uploading progress for one session's attachment can be transiently reset to nothing whenever any other session's attachment set changes.
🐛 Proposed fix — scope cleanup to only this session's stale ids
api.onChanged(({ sessionId, attachments }) => {
set((state) => {
- // Drop progress entries for attachments that finished staging.
+ // Drop progress entries for THIS session's attachments that finished
+ // staging — other sessions' in-flight progress must be untouched.
const uploading = new Set(
attachments.filter((a) => a.status === 'uploading').map((a) => a.id),
);
+ const sessionIds = new Set((state.bySession[sessionId] ?? []).map((a) => a.id));
const progress: Record<string, number> = {};
for (const [id, pct] of Object.entries(state.progress)) {
- if (uploading.has(id)) progress[id] = pct;
+ if (!sessionIds.has(id) || uploading.has(id)) progress[id] = pct;
}
return {
bySession: { ...state.bySession, [sessionId]: attachments },
progress,
};
});
});📝 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.
| hydrate: () => { | |
| const api = window.limboo?.attachment; | |
| if (!api || get().hydrated) return; | |
| api.onChanged(({ sessionId, attachments }) => { | |
| set((state) => { | |
| // Drop progress entries for attachments that finished staging. | |
| const uploading = new Set( | |
| attachments.filter((a) => a.status === 'uploading').map((a) => a.id), | |
| ); | |
| const progress: Record<string, number> = {}; | |
| for (const [id, pct] of Object.entries(state.progress)) { | |
| if (uploading.has(id)) progress[id] = pct; | |
| } | |
| return { | |
| bySession: { ...state.bySession, [sessionId]: attachments }, | |
| progress, | |
| }; | |
| }); | |
| }); | |
| api.onProgress(({ id, percent }) => { | |
| set((state) => ({ progress: { ...state.progress, [id]: percent } })); | |
| }); | |
| set({ hydrated: true }); | |
| }, | |
| hydrate: () => { | |
| const api = window.limboo?.attachment; | |
| if (!api || get().hydrated) return; | |
| api.onChanged(({ sessionId, attachments }) => { | |
| set((state) => { | |
| // Drop progress entries for THIS session's attachments that finished | |
| // staging — other sessions' in-flight progress must be untouched. | |
| const uploading = new Set( | |
| attachments.filter((a) => a.status === 'uploading').map((a) => a.id), | |
| ); | |
| const sessionIds = new Set((state.bySession[sessionId] ?? []).map((a) => a.id)); | |
| const progress: Record<string, number> = {}; | |
| for (const [id, pct] of Object.entries(state.progress)) { | |
| if (!sessionIds.has(id) || uploading.has(id)) progress[id] = pct; | |
| } | |
| return { | |
| bySession: { ...state.bySession, [sessionId]: attachments }, | |
| progress, | |
| }; | |
| }); | |
| }); | |
| api.onProgress(({ id, percent }) => { | |
| set((state) => ({ progress: { ...state.progress, [id]: percent } })); | |
| }); | |
| set({ hydrated: true }); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/stores/useAttachmentStore.ts` around lines 52 - 75, The cleanup
in useAttachmentStore’s hydrate/onChanged flow is too broad because it rebuilds
the global progress map from only the current session’s attachments, which can
erase progress for unrelated sessions. Update the onChanged handler to scope
removal to stale ids for the specific sessionId being updated: preserve any
existing progress entries not associated with that session, and only drop
progress for attachment ids in that session that are no longer uploading while
still merging the new bySession state.
…nd what I changed: Two separate things: 1. **Token wasn't reaching the publish shell** — `setx` only affects *newly opened* terminals, so the session where you ran `npm run publish` never had `GH_TOKEN`. 2. **Version collision** — `package.json` was the placeholder **1.2.5**, which already has a *full release* on GitHub. electron-builder defaults to publishing a *draft*, and GitHub refuses `draft` over an existing `release` → `existing type not compatible`. **Token now loads in any shell (no secret committed)** - Created `.env` (root) with `GH_TOKEN` — confirmed **gitignored** (`git check-ignore` → `.gitignore:61`, and it's absent from `git status`, so it can never be committed/pushed to the mirrors). - [scripts/dist.mjs](scripts/dist.mjs) now loads `.env` into the environment before electron-builder runs, but **only fills keys that aren't already set**, so CI's own `GH_TOKEN` always wins and nothing changes for CI. **Version → 1.3.4, tag auto-creates, no type conflict** - Bumped to **1.3.4** using the repo's own [ci/scripts/apply-tag-version.mjs](ci/scripts/apply-tag-version.mjs) (updated both `package.json` and `package-lock.json`) — the next version after your latest tag `v1.3.3`. I verified `v1.3.4` has **no** existing tag or GitHub release, so it publishes clean. - [electron-builder.yml](electron-builder.yml:26): added **`releaseType: release`**. This publishes straight to a full GitHub Release (which **auto-creates the `v1.3.4` tag**) instead of a draft, matching your existing releases (all `draft=false`) — so the type mismatch can't recur, and the auto-generated tag `v1.3.4` stays in lockstep with `package.json`. Version is now the single source of truth: bump it (or run `node ci/scripts/apply-tag-version.mjs vX.Y.Z`) and electron-builder generates the matching `v<version>` tag automatically. Just run it — in **any** terminal now: ``` npm run publish ``` It will build v1.3.4, then create the GitHub Release `v1.3.4` (auto-tagging), uploading `Limboo-Setup-1.3.4-x64.exe` + `latest.yml`. I didn't run it myself since it's an outward-facing action that creates a public release — that's yours to trigger.⚠️ **Still important:** that PAT is now in this chat and in `.env` on disk. Once publishing works, **regenerate it** on GitHub and update `.env` — treat the pasted one as compromised. One reminder from earlier: the `voice:warm` handler needs a full **`npm start` restart** to register (main-process IPC only reloads on restart) — that also activates the voice lag fixes.
1 participant
…ing the AttachmentManager, DB migration, IPC surface, agent injection (manifest +
additionalDirectories+ image content blocks), composer UI with the circular progress ring, and the settings section. I'll review its output against the code and write the final plan when it reports back.Summary
Type of change
Architectural reasoning
Verification
npm run lintpassesnpx vite build --config vite.renderer.config.mtspassesnpm start(for main/preload changes)Security checklist (if touching main process)
shell: true)contextIsolation/sandbox/ CSP / navigation lockdownDocumentation and changelog
docs/CHANGELOG.mdentry (if user-facing)Theme discipline (for UI changes)
dark:variants, no gradients)Note
Medium Risk
Touches agent permission boundaries (staging-dir read carve-out) and main-process file validation/staging; changes are defense-in-depth but warrant careful review of
canUseTooland path guards.Overview
Adds a full Attachment Manager so users can attach files from the composer (picker, drag-and-drop, image paste), with session-scoped staging, SQLite metadata (schema v9), and a new
window.limboo.attachmentIPC surface.Agent integration extends
agent:sendwith optional attachment IDs, appends a per-turn<attachments>manifest, grants read-only SDK access to the session staging dir (additionalDirectories+ acanUseToolcarve-out), sends raster images as vision blocks when enabled, and marks chips read when the agent opens a staged file.UI includes composer chips with staging progress, read-only chips on sent turns, Settings › Attachments (
settings.attachments,SETTINGS_VERSION12), and documentation updates.Also ships voice latency improvements (
voice:warm, deferred STT load, parallel mic capture with buffering) and release tooling tweaks (npm run make/publish→ dist,.envload indist.mjs,releaseType: releasein electron-builder). Version bump to 1.3.4.Reviewed by Cursor Bugbot for commit d3d6ba2. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes