Feat/p0 usefulness first - #41
Merged
Merged
Conversation
…truth The workflow ENV-sync now keeps only the 10 most recent /opt/smartsht/.env.bak-gha-* backups instead of accumulating them unbounded. Also documents that the GitHub ENV secret is authoritative (every auto-deploy overwrites the server .env from it), warns against hand-editing the live file, and notes how to verify the loaded env via /health or the boot log.
Move the keep-last-10 prune outside the [ -f /opt/smartsht/.env ] guard so stale backups are pruned on every sync, including runs where the live .env file does not exist. Pruning depends only on the .bak-gha-* files, not the live file.
Make imports honest, block gapped apply_formula without preview, keep critical insights visible, and document the useful-first strategy.
Reviewer's GuideThis PR completes the usefulness-first P0 foundation by documenting the roadmap, making XLSX import limitations visible, adding guarded formula application with previews, improving audit activation behavior, capturing bounded feedback context, and extending real Formualizer parity tests. Sequence diagram for honest workbook importsequenceDiagram
participant User
participant Toolbar
participant XLSX as importWorkbookFromFileWithMeta
participant Store as importWorkbook
participant Effects as applyWorkbookImportEffects
participant UI as ImportInsightsOverlay
User->>Toolbar: Select workbook
Toolbar->>XLSX: importWorkbookFromFileWithMeta(file)
XLSX-->>Toolbar: workbook, meta.warnings
Toolbar->>Store: importWorkbook(workbook, fileName, warnings)
Store->>Effects: applyWorkbookImportEffects(workbook, meta)
Effects-->>UI: Import message and warning toast
UI-->>User: Show insights and import limitations
Sequence diagram for guarded formula applicationsequenceDiagram
participant Agent
participant Preview as buildActionPreview
participant Handler as handleApplyFormula
participant Risk as detectApplyFormulaRangeGapRisk
participant Sheet
Agent->>Preview: buildActionPreview(apply_formula, params)
Preview-->>Agent: Proposed cell change
Agent->>Handler: handleApplyFormula(params, ctx, sheet)
Handler->>Risk: detectApplyFormulaRangeGapRisk(formula, sheet, getComputedValue, cell)
alt Range gap risk and confirmGaps not set
Risk-->>Handler: Warning
Handler-->>Agent: Blocked apply_formula
else No risk or confirmGaps set
Risk-->>Handler: null or warning
Handler->>Sheet: setCellValue(cell, null, formula)
Handler-->>Agent: Successful apply result
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (21)
📝 WalkthroughWalkthroughThis PR adds formula previews and range-gap checks, improves XLSX import warnings and audit-panel behavior, adds context to chat feedback telemetry, expands Formualizer parity tests, and updates strategy and engine-gap documentation. ChangesFormula Application
Workbook Import Feedback
Chat Feedback Context
Formula Engine Compatibility
Forward Plan Documentation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant XLSXImporter as XLSX importer
participant Toolbar
participant ImportWorkbook as importWorkbook
participant ImportOrchestration as applyWorkbookImportEffects
participant Chat
participant Toast
XLSXImporter->>Toolbar: workbook and import warnings
Toolbar->>ImportWorkbook: workbook and metadata
ImportWorkbook->>ImportOrchestration: workbook and metadata
ImportOrchestration->>Chat: append import honesty note
ImportOrchestration->>Toast: show first warning
✨ Finishing Touches📝 Generate docstrings
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/agent/toolHandlers/columnOps.ts" line_range="172-174" />
<code_context>
+ return Number.isFinite(num) && computed.trim() !== '' ? num : null
+}
+
+function confirmGapsRequested(params: Record<string, unknown>): boolean {
+ return params.confirmGaps === true || params.confirmGaps === 'true' || params.force === true
+}
+
/** Apply a formula below the last data row in a column. */
</code_context>
<issue_to_address>
**issue (bug_risk):** A gapped `apply_formula` action that has a generated preview is still rejected when the user clicks Apply, because the preview does not add `confirmGaps` or `force` to the action parameters and the executor calls `handleApplyFormula` unchanged. The only way through is for the model to pre-populate an override, so the user cannot confirm the displayed warning through the normal Apply flow.
**Triggers:** When an aggregate formula excludes an adjacent numeric cell and the action is generated without `confirmGaps: true`.
**Suggested fix:** Treat an explicit Apply after displaying the preview as confirmation, or provide a confirmation path that sets `confirmGaps: true` before executing the action.
</issue_to_address>
### Comment 2
<location path="src/io/xlsx.ts" line_range="214-216" />
<code_context>
+ if (style.fgColor || style.bgColor || style.fill || style.font || style.border || style.alignment)
+ return true
+ if (style.numFmt && typeof style.numFmt === 'object') return true
+ // patternType alone (e.g. "none") is a default stub, not real styling
+ const keys = Object.keys(style).filter((k) => k !== 'patternType')
+ return keys.length > 0
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** A workbook containing only number-format metadata is classified as having meaningful style objects, but `visualStylesApplied` remains zero because number formats are not included in `visualKeys`. Import therefore emits the misleading warning that no visual styles could be applied even though the style metadata was valid and intentionally non-visual.
**Triggers:** When an imported workbook uses number formats without fills, fonts, borders, or other visual styles.
**Suggested fix:** Track number-format application separately or exclude number-format-only objects from the visual-style warning condition.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and the deployment sync now permanently deletes all but the ten newest production .env backups, so reverting the change cannot restore backups already pruned. The feedback change also sends up to 200 characters of assistant content through telemetry, which could expose workbook-derived information and cannot be undone after transmission.
Blocking findings: src/agent/toolHandlers/columnOps.ts:174, src/io/xlsx.ts:216
…umber-format-only imports - applyAction: treat explicit Apply of a previewed apply_formula as gap confirmation (confirmGaps=true) so reviewed formulas are not rejected - xlsx import: track numberFormatsApplied so number-format-only workbooks no longer trigger the misleading 'no visual styles applied' warning
Ocean82
added a commit
that referenced
this pull request
Sep 27, 2026
…reakage Concludes the merge of 9e68548 (Feat/p0 usefulness first, #41). HEAD already contained all of #41's content, so the merge itself is a no-op; resolved by keeping our side, which is a strict superset: P0.3 gap detection extracted to src/lib/formulaGapRisk.ts and wired into the preview path, P0.5 user-entered thumbs-down detail, and P1.4 style_recipe preview support. Fixes four pre-existing breakages on this branch, masked until now by the unresolved conflict markers: - previewBuilders: add the missing detectFormulaRangeGapRisk import; the P0.3 preview path did not compile - styleRecipes.test: use refToCell(row, col) -- cellToRef takes a cell id - parser: add style_recipe and format_as_table to the question-veto set, so "Should I add a total row?" no longer fires a bulk restyle - styleRecipes: drop the dead totalsRow binding, which failed lint:ci (--max-warnings=0) Verified green: lint:ci, typecheck, 1822 unit tests, 344 server tests, 12 realengine golden-set tests.
Ocean82
added a commit
that referenced
this pull request
Sep 27, 2026
…eedback (#43) Completes the remaining P0 (Useful First) items. P0.1, P0.2 and P0.4 already landed on main via #41; this covers the rest. P0.3 — act-path safety The range-gap detector ("SUM skips an adjacent numeric cell") moves out of columnOps.ts into src/lib/formulaGapRisk.ts so the Apply/Reject *preview* path can share it, not just the execute path. The preview now surfaces the risk as a warning alongside the proposed changes, and Apply confirms those warnings via the existing confirmGaps override. columnOps.ts shrinks by ~110 lines as a result. P0.5 — AI quality loop Thumbs-down now opens an optional "what went wrong?" field. The rating is recorded immediately on click; submitting the note re-records it with the user's own words instead of the auto-derived fingerprint, which is what makes failover analysis actionable. Cmd/Ctrl+Enter submits, Escape skips. Parser question-veto format_as_table joins DESTRUCTIVE_TOOLS so "should I format this as a table?" no longer fires a bulk restyle. Targeted tools stay excluded — the polite-framing path deliberately treats "can you highlight X" as a command. Verified: lint:ci, typecheck, 1791 unit tests, 344 server tests, 12 realengine golden-set tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Complete the usefulness-first P0 by improving spreadsheet import transparency, safe formula edits, audit visibility, and quality feedback while documenting the next formatting-focused priorities.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation