drag and drop for import table#382
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a multi-file import flow for customized table input methods: ChangesImport Customized Table Feature and Tests
Submodule Pointer Updates
Sequence Diagram(s)sequenceDiagram
participant User
participant ImportTableView
participant ImportTableVM
participant FileSystem
participant Fcitx
User->>ImportTableView: drag/click to select files
ImportTableView->>ImportTableVM: addFiles(urls)
ImportTableVM->>ImportTableVM: filter suffixes, deduplicate, append to selectedFiles
ImportTableView-->>User: renders removable file list
User->>ImportTableView: click Import
ImportTableView->>ImportTableVM: convertTxt(selectedFiles)
ImportTableVM-->>ImportTableView: [failedTxtNames]
ImportTableView->>ImportTableVM: importFiles()
ImportTableVM->>FileSystem: create dirs, copy files
FileSystem-->>ImportTableVM: results / errors
ImportTableVM-->>ImportTableView: [importedBaseNames]
ImportTableView->>Fcitx: reloadConfig()
ImportTableView->>ImportTableView: compute newIMs, call onAdd / onError / finalize
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 `@appium/test_import_customized_table.py`:
- Line 19: The table_path variable construction at line 19 relies on os.getcwd()
which makes the test fragile and dependent on the execution context. Replace the
current path construction that uses os.getcwd() with one that uses __file__ to
get the directory of the current test file itself, then navigate relatively from
there to reach the "appium/table" directory. This ensures the path is always
resolved correctly regardless of the working directory when the test runs.
In `@appium/util/window.py`:
- Around line 67-80: The scrollTo function contains an unbounded while True loop
that can hang indefinitely if the element with the given ID is never found
during scrolling. Add a bounded exit mechanism by introducing a counter variable
to track the number of scroll attempts, and break from the loop after reaching a
maximum attempt limit. When the limit is exceeded, either raise an appropriate
exception or handle the case gracefully to prevent the test from hanging.
- Around line 19-30: The find_element_by_id function currently catches all
exceptions and converts them to a generic timeout message, which masks
actionable failures like the ValueError raised when duplicate elements are found
on line 27. Replace the broad except Exception block with a specific exception
handler that only catches the timeout exception (TimeoutException from
selenium.common.exceptions), allowing other exceptions like the ValueError for
duplicate matches to propagate with their original error messages. This
preserves meaningful error information while still providing a timeout-specific
message when the wait condition actually times out.
In `@src/config/ImportTable.swift`:
- Around line 211-227: The convertTxt function returns failures, but the code
currently reports these failures only after performing the import operations
including importTableVM.importFiles(), Fcitx.reload(), and onAdd(newIMs), which
leaves the system in a partial import state. Move the failure check immediately
after the convertTxt call at the beginning of the block, construct the error
message if failures are not empty, call onError with that message, and return
early before proceeding with importTableVM.importFiles(), importTableVM.load(),
filtering, Fcitx.reload(), onAdd, and dismiss operations. This ensures the
import process fails fast and does not proceed if txt conversion failed.
- Around line 83-85: The conversion operation at the exec call (where the
converter is invoked with url.localPath() and dest.localPath()) lacks protection
against overwriting existing destination files. Before executing the converter,
check whether the destination file (stored in the dest variable which is created
from tableLocalDir.appendingPathComponent) already exists, and handle this case
appropriately by either skipping the conversion, raising an error, or using an
explicit overwrite flag to prevent unintended data loss or ambiguous converter
failures.
In `@src/config/util.swift`:
- Around line 6-7: In the ProcessInfo.processInfo.environment lookup for FCITX_*
environment variables, the current code accepts empty string values and creates
a path from them, which bypasses the fallback logic. Modify the condition that
checks if let value exists to also verify that the value is not empty
(value.isEmpty is false), ensuring that empty environment variables are treated
as unset and allow the fallback to be used instead of creating a URL from an
empty string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e3cd242-70f1-4ce6-9bc2-1cb573663b10
📒 Files selected for processing (12)
appium/table/customized.conf.inappium/table/customized.dictappium/table/customized.txtappium/test_import_customized_table.pyappium/util/file.pyappium/util/window.pyassets/zh-Hans.lproj/Localizable.stringsassets/zh-Hant.lproj/Localizable.stringsfcitx5src/config/ImportTable.swiftsrc/config/SelectInputMethod.swiftsrc/config/util.swift
arm64 comparisonNo difference. x86_64 comparisonNo difference. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit